Skip to content

drivers: mdio_esp32: let the REF_CLK be initialized before the PHY. #75024

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

Merged

Conversation

iwasz
Copy link
Contributor

@iwasz iwasz commented Jun 26, 2024

When GPIO17 or 16 is used as an external REF_CLK signal, the output is enabled in eth_esp32.c This was added in PR number #65759 (by @bbilas ) and then refined in PR #74442 (by @sylvioalves). However this does not work for PHYs which need the REF_CLK for MDIO communication, such as LAN8720A. In such cases phy_mii driver tries to get the ID of such a PHY before REF_CLK is present. Therefore in this PR I propose to move REF_CLK initialization from eth_esp32.c to mdio_esp32.c which gets initialized before PHY and ETH.

There's a topic about this as well: #68907

Copy link

Hello @iwasz, 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. 😊

@marekmatej
Copy link
Collaborator

Hi @iwasz, any reason why the original location of the clock init remains the same? eth_esp32.c:282

@iwasz
Copy link
Contributor Author

iwasz commented Jun 26, 2024

why the original location of the clock init remains the same? eth_esp32.c:282

Hey! I was unsure what to do about it. There's an #if DT_INST_NODE_HAS_PROP(0, ref_clk_output_gpios) there which decides if we want enable RMII input or output. If I remove the whole "true case" block that I moved to mdio_esp32.c then we would be left with something like:

#if !DT_INST_NODE_HAS_PROP(0, ref_clk_output_gpios)
		emac_hal_iomux_rmii_clk_input();
		emac_ll_clock_enable_rmii_input(dev_data->hal.ext_regs);
#endif

And then having ref-clk-output-gpios defined in the eth (i mean here dts/bindings/ethernet/espressif,esp32-eth.yaml) makes little sense - it would be used kinda like a boolean flag even though it's a phandle-array.

@cbrake
Copy link

cbrake commented Jun 26, 2024

This PR gets Ethernet working on the Olimex ESP32_POE boards.

My current setup:

https://github.com/simpleiot/zephyr-siot

https://github.com/simpleiot/zephyr-siot/blob/main/boards/esp32_poe/esp32_poe_procpu.dts#L169

Note, ref-clk-output-gpios = <&gpio0 17 0>; is required in both the mdio and eth sections, otherwise the mdio driver sets up the clock and the phy is detected, but then the eth driver shuts it off.

When GPIO17 or 16 is used as an external REF_CLK signal, the output is
enabled in eth_esp32.c This was added in PR number zephyrproject-rtos#65759 and then refined
in PR zephyrproject-rtos#74442. However this does not work for PHYs which need the REF_CLK
for MDIO communication, such as LAN8720A. In such cases phy_mii driver
tries to get the ID of such a PHY before REF_CLK is present. Therefore
in this PR I propose to move REF_CLK initialization from eth_esp32.c to
mdio_esp32.c which gets initialized before PHY and ETH.

Signed-off-by: Łukasz Iwaszkiewicz <[email protected]>
@iwasz iwasz force-pushed the initialize_ref_clk_in_mdio_esp32 branch from 19fcf13 to 0207c62 Compare July 25, 2024 11:27
@decsny decsny removed their request for review July 29, 2024 20:02
@MaureenHelm
Copy link
Member

@sylvioalves please take a look

bbilas
bbilas previously requested changes Nov 12, 2024
Copy link
Collaborator

@bbilas bbilas left a comment

Choose a reason for hiding this comment

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

Those changes do not look correct. That is copied and pasted from the eth_esp32 driver. @sylvioalves, could you take a look at it?

@kartben
Copy link
Collaborator

kartben commented Dec 19, 2024

@sylvioalves wonder if you can have a look so we can unblock this

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Feb 18, 2025
@cbrake
Copy link

cbrake commented Feb 24, 2025

Hi, we still need this fix. Is there any chance this can be merged, or an alternate solution worked on?

Copy link
Collaborator

@bbilas bbilas left a comment

Choose a reason for hiding this comment

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

unblock

@bbilas bbilas dismissed their stale review April 23, 2025 10:51

unblock

@kartben kartben merged commit 2fa0afb into zephyrproject-rtos:main Apr 23, 2025
22 checks passed
Copy link

Hi @iwasz!
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
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: MDIO platform: ESP32 Espressif ESP32
Projects
None yet
Development

Successfully merging this pull request may close these issues.