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

drivers: ethernet: w5500: Add link status detection #68752

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

vshymanskyy
Copy link
Contributor

@vshymanskyy vshymanskyy commented Feb 8, 2024

This PR introduces link status detection functionality to the W5500 Ethernet driver within the Zephyr OS. This feature enhances the driver's capability to monitor and respond to changes in Ethernet link status (cable insertion/removal), providing a more robust and reliable network connection for devices utilizing the W5500 chip.

  • Implemented link status detection using the W5500's built-in register functions.
  • Added periodic link status polling to the Ethernet driver, configurable via Kconfig.
  • Speeds up DHCPv4 configuration (from 9 seconds at best, to ~2 seconds after power-up).
  • Should be completely backward compatible.

Test

The test was performed on WeAct BlackPill STM32F411CE + WIZnet W5500

*** Booting Zephyr OS build v3.6.0-rc1-8-gf6ecad20a1e7 ***
[00:00:00.006,000] <inf> eth_w5500: W5500 Initialized
[00:00:00.007,000] <inf> sample: Started
[00:00:00.030,000] <inf> eth_w5500: w5500@0 MAC set to 01:02:03:04:05:06
[00:00:00.030,000] <inf> sample: DHCPv4 start on w5500@0
[00:00:02.007,000] <inf> eth_w5500: w5500@0: Link up
[00:00:02.018,000] <inf> net_dhcpv4: Received: 10.42.0.211
<unplug cable>
[00:00:12.552,000] <inf> eth_w5500: w5500@0: Link down
<plug in cable>
[00:00:17.053,000] <inf> eth_w5500: w5500@0: Link up

Copy link

github-actions bot commented Feb 8, 2024

Hello @vshymanskyy, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@zephyrbot zephyrbot requested a review from decsny February 8, 2024 14:03
@vshymanskyy vshymanskyy force-pushed the w5500_link_status branch 4 times, most recently from 260d3f8 to d7629d6 Compare February 8, 2024 21:16
@parthitce parthitce requested review from jukkar and rlubos February 9, 2024 06:26
drivers/ethernet/eth_w5500.c Outdated Show resolved Hide resolved
drivers/ethernet/eth_w5500.c Outdated Show resolved Hide resolved
decsny
decsny previously requested changes Feb 19, 2024
Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

It's completely normal for the current phy drivers to have a monitor period to poll the status of the link, in this case this hardware is a combination of a phy and mac, so I think it is fair for it to do the same thing. However, please use the kconfig PHY_MONITOR_PERIOD which exists in the drivers/ethernet/phy/Kconfig file, as this is the common kconfig which users will expect to use for this purpose. Also, I suggest to use the system workqueue instead of creating a new thread, I believe this would improve the memory footprint.

@decsny decsny self-assigned this Feb 19, 2024
@decsny
Copy link
Member

decsny commented Feb 19, 2024

@nashif This PR did not get an assignee, is that a mistake by the bot? I assigned myself for now

@nashif
Copy link
Member

nashif commented Feb 19, 2024

@nashif This PR did not get an assignee, is that a mistake by the bot? I assigned myself for now

its not a mistake, the area has no maintainer.

@decsny
Copy link
Member

decsny commented Feb 19, 2024

@nashif This PR did not get an assignee, is that a mistake by the bot? I assigned myself for now

its not a mistake, the area has no maintainer.

okay, I didn't know that's how it works, thanks

@vshymanskyy
Copy link
Contributor Author

Removing the thread is a more significant change that is out of scope of this PR, I believe.

@decsny
Copy link
Member

decsny commented Feb 20, 2024

Removing the thread is a more significant change that is out of scope of this PR, I believe.

Okay that's fine, because it already existed, but it was just a suggestion

Still try to use the common kconfig if you can

@decsny
Copy link
Member

decsny commented Feb 20, 2024

@parthitce please return to this PR and note that it is not unusual for polling phy status in current zephyr drivers

@parthitce
Copy link
Member

@parthitce please return to this PR and note that it is not unusual for polling phy status in current zephyr drivers

How about introducing struct ethphy_driver_api for w5500 with the needed ops to handle it instead of custom way of handling it?

@decsny
Copy link
Member

decsny commented Feb 22, 2024

@parthitce please return to this PR and note that it is not unusual for polling phy status in current zephyr drivers

How about introducing struct ethphy_driver_api for w5500 with the needed ops to handle it instead of custom way of handling it?

Its not a bad idea but it seems outside the scope of this PR because it's not required to add link detection to the current driver.

@parthitce
Copy link
Member

@parthitce please return to this PR and note that it is not unusual for polling phy status in current zephyr drivers

How about introducing struct ethphy_driver_api for w5500 with the needed ops to handle it instead of custom way of handling it?

Its not a bad idea but it seems outside the scope of this PR because it's not required to add link detection to the current driver.

Agreed!

Copy link
Member

@parthitce parthitce left a comment

Choose a reason for hiding this comment

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

LGTM, please fix the CI.

Implemented link status detection using the W5500 built-in registers.
Added periodic link status polling, configurable via Kconfig.
Speeds up DHCPv4 from 9 seconds at best, to ~2 seconds after power-up.

Signed-off-by: Volodymyr Shymanskyy <[email protected]>
@vshymanskyy
Copy link
Contributor Author

Not sure why CI was broken, I just pushed a dummy update to restart it

@vshymanskyy vshymanskyy requested a review from decsny February 29, 2024 14:14
@vshymanskyy
Copy link
Contributor Author

All checks have passed

@aescolar aescolar merged commit 77fc3ea into zephyrproject-rtos:main Mar 1, 2024
22 checks passed
Copy link

github-actions bot commented Mar 1, 2024

Hi @vshymanskyy!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants