Introducing stm32 MDIO driver #71012

spectrum70 · 2024-04-02T19:36:26Z

Introducing stm32 MDIO driver, needed for all ethernet phy drivers on top of it, as actually needed from adin1100.

MDIO hw module is part of the ETH IP module in both stm32 h5/h7 families (and also likely f, not interested from this patch-set).
This patch-set abstracts-out the MDIO related registers control, in a separate MDIO driver, so that further PHY drivers can be implemented, and MDIO and PHY apis can be used.

Below a simple unit test, where i write and read registers, testing the contents with previously written values:

[00:00:00.001,000] <inf> phy_mii: PHY (0) ID 283BC81

[00:00:00.002,000] <inf> phy_mii: PHY (0) 2.4V mode supported
[00:00:00.002,000] <inf> phy_mii: PHY (0) 2.4V mode disabled
[00:00:00.002,000] <inf> phy_mii: PHY (0) Link is up
[00:00:00.002,000] <inf> phy_mii: PHY (0) Link speed 10 Mb, full duplex

[00:00:00.003,000] <err> net_if: Cannot join all nodes address ff02::1 for 1 (-115)
[00:00:00.003,000] <err> net_if: Cannot join solicit node address ff02::1:ff67:9f3 for 1 (-115)
*** Booting Zephyr OS build v3.6.0-2008-g7a2ec098c980 ***
[00:00:00.003,000] <inf> net_config: Initializing network
[00:00:00.003,000] <inf> net_config: Waiting interface 1 (0x2400138c) to be up...
[00:00:00.503,000] <inf> net_config: Interface 1 (0x2400138c) coming up
[00:00:00.503,000] <inf> net_config: IPv4 address: 192.168.5.4
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 MDIO read ...
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 MDIO read -ok-
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 MDIO write ...
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 MDIO write -ok-
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 PHY read ...
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 PHY read -ok-
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 PHY write ...
[00:00:00.503,000] <dbg> net_stats_sample: main: C22 PHY write -ok-
[00:00:01.503,000] <dbg> net_stats_sample: main: Getting link state, should be up now ...
[00:00:01.503,000] <dbg> net_stats_sample: main: Link state ... UP
[00:00:01.503,000] <dbg> net_stats_sample: main: Test completed \o/
uart:~$

Waiting for comments.
Thanks

gramsay0 · 2024-04-04T08:42:24Z

Do you need to remove all the phy related stuff from the STM32 Ethernet drivers?
Is it possible to extend the existing phy_adin2111.c to support adin1100 (it looks like Linux combines them https://github.com/torvalds/linux/blob/master/drivers/net/phy/adin1100.c#L344)

edit: that is it seems like the phy would be managed by both the phy and Ethernet drivers (and without synchronisation)

spectrum70 · 2024-04-04T12:34:13Z

Hi @gramsay0

thanks for the feedbacks,

Do you need to remove all the phy related stuff from the STM32 Ethernet drivers? Is it possible to extend the existing phy_adin2111.c to support adin1100 (it looks like Linux combines them
https://github.com/torvalds/linux/blob/master/drivers/net/phy/adin1100.c#L344)

Well, adin2111/1110 are spi-based only, adin1100 (and adin1300 that will follow) are rmii interface based, and industrial ethernet stuff. A PHY driver for them is asked by ADI, since generic-phy (that i tested) is not working properly for them.

edit: that is it seems like the phy would be managed by both the phy and Ethernet drivers (and without synchronisation)

Yes, correct, if it's an issue i can remove PHY HAL access for H5/H7 from eth_stm32_hal.c in case, and use a MDIO API call in that place.

gramsay0 · 2024-04-05T02:57:08Z

Well, adin2111/1110 are spi-based only, adin1100 (and adin1300 that will follow) are rmii interface based, and industrial ethernet stuff. A PHY driver for them is asked by ADI, since generic-phy (that i tested) is not working properly for them.

phy_adin2111.c does not use SPI, it uses the MDIO API which could be implemented by any MDIO driver.
mdio_adin2111.c calls directly into eth_adin2111.c functions which use SPI.

So it should be possible to use phy_adin2111.c (possibly with extensions for adin1100) with your stm32 MDIO driver.

Yes, correct, if it's an issue i can remove PHY HAL access for H5/H7 from eth_stm32_hal.c in case, and use a MDIO API call in that place.

Someone more familiar with this area would be best to give you advice on this

drivers/mdio/Kconfig.stm32_hal

drivers/mdio/mdio_stm32_hal.c

decsny · 2024-04-05T03:28:23Z

drivers/mdio/mdio_stm32_hal.c

+
+	k_sem_take(&dev_data->sem, K_FOREVER);
+
+	ret = HAL_ETH_WritePHYRegister(hmdio, prtad, regad, data);


I noticed that the stm32 hal looks for definition of ETH_MDIO_BUS_TIMEOUT to decide how long to block and I am wondering if it would be worth any value to provide this definition based on the value of a kconfig so that it is configurable

It is,

#define ETH_MDIO_BUS_TIMEOUT 1000U

So it's actually 1 second max. It's a max limit, so busy flag is cleared to 0 much before. Seems not simple for me to replace this value, since part of stm32cube module repo.

you could make a kconfig and give zephyr_compile_definitions in the cmake based on that config, but if you don't think this is worth it then I don't think it is a big deal to skip it in this PR

drivers/mdio/mdio_stm32_hal.c

decsny · 2024-04-05T03:33:28Z

drivers/ethernet/phy/Kconfig

+	depends on DT_HAS_ADI_ADIN1100_PHY_ENABLED
+	depends on ETH_STM32_HAL
+	depends on MDIO
+	depends on MDIO_ST_STM32_HAL


this should not be dependent on the implementations being stm32 hal, in principle the phy driver should be able to fit with any mdio controller or ethernet device

done, thanks, removed both MDIO_ST_STM32_HAL and ETH_STM32_HAL, since stm32 mdio driver goes trough stm32 HAL.

it seems they are still there, is it intentional

decsny · 2024-04-05T03:40:37Z

drivers/ethernet/phy/phy_adin1100.c

the other reviewer is right that the most important consumer of the phy abstraction layer is the ethernet mac driver itself, and honestly it would not be a good idea to have a separate phy driver while having basically another phy driver embedded/hardcoded per se in the mac driver, they could conflict without knowing about each other... I havent looked at the stm32 driver but if it really does use the stm hal phy calls then it would probably be in your interest to convert it to using the zephyr phy api and go through this driver (or whatever phy driver, since that is the idea)

@decsny and all, thanks for all the feedbacks,

so, based on the above suggestions, i would go in this direction now for a v2:

see if i can use same phy driver for adin2111 and adin1100

on the stm32 eth driver, will use the Zephyr phy api to avoid conflicts

keeping/fixing all your suggested points above

Let me know if you see any issue.

Thanks a lot

spectrum70 · 2024-04-09T06:29:30Z

@decsny

As of now, cannot use adin2111/1110 phy driver for adin1100, since several accesses are Clause 45, and stm32 mdio cannot implement it (missing HAL functions). I pushed a PR to ST HAL for this.

STMicroelectronics/stm32h7xx_hal_driver#52

So right now, only way for me to proceed is to keep in adin1100 phy driver. If ST HAL will be accepted, i can cleanup code removing the phy driver.

gramsay0 · 2024-04-09T07:41:26Z

As of now, cannot use adin2111/1110 phy driver for adin1100, since several accesses are Clause 45, and stm32 mdio cannot implement it (missing HAL functions). I pushed a PR to ST HAL for this.

STMicroelectronics/stm32h7xx_hal_driver#52

So right now, only way for me to proceed is to keep in adin1100 phy driver. If ST HAL will be accepted, i can cleanup code removing the phy driver.

Both adin2111 and adin1110 support clause 22 and 45. So writing an adin1110 phy driver just to support clause 22 (because the existing adin2111 uses clause 45) does not seem like a good path forward. IMO a Kconfig to enable C45 support would seem more appropriate.

Although adding clause 45 support to the STM HAL is a great alternative too 👍

spectrum70 · 2024-04-09T08:20:55Z

@gramsay0

thanks,

well, ADI naming is quite funny :) there are ADIN2111/1110 (same eth/mdio/phy driver, spi based) and this new ADIN1100 that is rmii. It requires right now a new driver since from there i can access MMD registers using the "bridge, indirect accerss" feature of it, simulating Clause 45, that cannot be implemented now in STM32 mdio. So for now keeping in adin1100-phy.

If ST guys will accept HAL changhes, i promise clean up and to remove the adin1100 phy driver.

gramsay0 · 2024-04-09T08:39:09Z

well, ADI naming is quite funny :) there are ADIN2111/1110 (same eth/mdio/phy driver, spi based) and this new ADIN1100 that is rmii. It requires right now a new driver since from there i can access MMD registers using the "bridge, indirect accerss" feature of it, simulating Clause 45, that cannot be implemented now in STM32 mdio. So for now keeping in adin1100-phy.

If a new variant supports a new feature, then you can add that to the device tree, and check / make use of the new feature in the driver, no need to duplicate the other functionality.

If ST guys will accept HAL changhes, i promise clean up and to remove the adin1100 phy driver.

👍

spectrum70 · 2024-04-09T14:18:39Z

well, ADI naming is quite funny :) there are ADIN2111/1110 (same eth/mdio/phy driver, spi based) and this new ADIN1100 that is rmii. It requires right now a new driver since from there i can access MMD registers using the "bridge, indirect accerss" feature of it, simulating Clause 45, that cannot be implemented now in STM32 mdio. So for now keeping in adin1100-phy.

If a new variant supports a new feature, then you can add that to the device tree, and check / make use of the new feature in the driver, no need to duplicate the other functionality.

If ST guys will accept HAL changhes, i promise clean up and to remove the adin1100 phy driver.

👍

Ack, will try to keep adin1110 phy with some conditionals for rmii

marwaiehm-st · 2024-04-30T10:18:44Z

Test ok on boards : nucleo_h743zi and stm32h573i_dk

spectrum70 · 2024-04-30T12:35:17Z

@marwaiehm-st thanks ! I tested this, but if you have a chance, as double check you may test still an ethernet FX, to be sure is not damaged/impacted from the change (same ETH driver).

drivers/mdio/mdio_stm32_hal.c

MDIO is part of the ETH IP, but some phy chip may need a specific phy driver to set up certain vendor registers enabling particular features. Add support for stm32 mdio access. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Add mdio node for h5 and h7 series. Since MDIO registers are part of the same ETH hw IP, keeping mdio node just as a child of mac/eth, cannot see as appropriate to assign an adddress to it. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Add mdio and phy nodes for h5/h7 families. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Introducing MDIO and PHY support for stm32, phy driver gets error (-116) if it tries to read phy chip id, since MDIO IP is part of ETH IP, and eth hw module is still not initialized. Forcing a priority that allows possibly connected PHY chip to be detected properly at initial boot. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Use PHY apis to access the PHY, to avoid any kind of collisions with other tasks using the PHY apis. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Add support for similar adin1100 phy, boath are 10Base-T1L, only difference is that adin1100 connects through r/mii. Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>

spectrum70 · 2024-05-02T09:12:06Z

Changes for v5:

fix missing braces on if statements

spectrum70 · 2024-05-03T08:51:39Z

@MaureenHelm @erwango so fixed last issue reported, could you please re-accept, if all ok ? Thanks.

erwango · 2024-05-03T09:01:44Z

@MaureenHelm @erwango so fixed last issue reported, could you please re-accept, if all ok ? Thanks.

I'd like @marwaiehm-st to confirm this has no impact on F7 series first. Otherwise it's fine for me.
This should be done in a couple of days max.

marwaiehm-st · 2024-05-06T08:02:50Z

Test ok on board nucleo_f767zi , there is no impact on F7 series first.

aescolar · 2024-05-07T07:42:17Z

@spectrum70 can you send a follow up PR adding a line to the release notes? Thanks in advance.

spectrum70 requested review from erwango, rlubos, tbursztyka, jukkar, ABOSTM and FRASTM as code owners April 2, 2024 19:36

zephyrbot added platform: STM32 ST Micro STM32 area: MDIO area: Devicetree Binding PR modifies or adds a Device Tree binding area: Ethernet platform: ADI Analog Devices, Inc. labels Apr 2, 2024

zephyrbot requested review from decsny, Desvauxm-st, galak, gautierg-st, GeorgeCGV, MaureenHelm and microbuilder April 2, 2024 19:37

zephyrbot assigned MaureenHelm Apr 2, 2024

erwango requested a review from marwaiehm-st April 3, 2024 07:24

decsny reviewed Apr 5, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/adin1100 branch from 7a2ec09 to 74aec57 Compare April 12, 2024 13:48

MaureenHelm previously approved these changes May 1, 2024

View reviewed changes

pdgendt reviewed May 2, 2024

View reviewed changes

drivers/mdio/mdio_stm32_hal.c Outdated Show resolved Hide resolved

drivers/mdio/mdio_stm32_hal.c Outdated Show resolved Hide resolved

Angelo Dureghello and others added 6 commits May 2, 2024 11:06

drivers: mdio: add stm32 mdio support

1d4d6bb

MDIO is part of the ETH IP, but some phy chip may need a specific phy driver to set up certain vendor registers enabling particular features. Add support for stm32 mdio access. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

boards: st: add mdio and phy nodes

ab29ee5

Add mdio and phy nodes for h5/h7 families. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

drivers: eth: eth_stm32: use phy api support for h5/h7

a96518d

Use PHY apis to access the PHY, to avoid any kind of collisions with other tasks using the PHY apis. Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

drivers: eth: phy: adin2111: add support for adin1100 phy

047024c

Add support for similar adin1100 phy, boath are 10Base-T1L, only difference is that adin1100 connects through r/mii. Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>

spectrum70 dismissed stale reviews from MaureenHelm and erwango via 047024c May 2, 2024 09:11

spectrum70 force-pushed the wip/bl/adin1100 branch from 863ade9 to 047024c Compare May 2, 2024 09:11

spectrum70 requested review from pdgendt, erwango and MaureenHelm May 2, 2024 11:34

decsny added the block: HW Test Testing on hardware required before merging label May 3, 2024

marwaiehm-st removed the block: HW Test Testing on hardware required before merging label May 6, 2024

erwango approved these changes May 6, 2024

View reviewed changes

marwaiehm-st approved these changes May 6, 2024

View reviewed changes

decsny approved these changes May 6, 2024

View reviewed changes

MaureenHelm approved these changes May 6, 2024

View reviewed changes

aescolar merged commit dc376a8 into zephyrproject-rtos:main May 7, 2024
25 checks passed

This was referenced May 7, 2024

samples/net/sockets/echo_server/ sample.net.sockets.echo_server.usbnet fails to build for stm32h747i_disco/stm32h747xx/m7 on main #72433

Closed

boards stm32h747i_disco: Add missing mdio node #72445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing stm32 MDIO driver #71012

Introducing stm32 MDIO driver #71012

spectrum70 commented Apr 2, 2024 •

edited

gramsay0 commented Apr 4, 2024 •

edited

spectrum70 commented Apr 4, 2024

gramsay0 commented Apr 5, 2024

decsny Apr 5, 2024

spectrum70 Apr 9, 2024

decsny Apr 15, 2024

decsny Apr 5, 2024

spectrum70 Apr 9, 2024

decsny Apr 15, 2024

decsny Apr 5, 2024

spectrum70 Apr 8, 2024 •

edited

decsny Apr 8, 2024

spectrum70 commented Apr 9, 2024

gramsay0 commented Apr 9, 2024

spectrum70 commented Apr 9, 2024 •

edited

gramsay0 commented Apr 9, 2024

spectrum70 commented Apr 9, 2024

marwaiehm-st commented Apr 30, 2024

spectrum70 commented Apr 30, 2024 •

edited

spectrum70 commented May 2, 2024

spectrum70 commented May 3, 2024

erwango commented May 3, 2024

marwaiehm-st commented May 6, 2024

aescolar commented May 7, 2024


		k_sem_take(&dev_data->sem, K_FOREVER);

		ret = HAL_ETH_WritePHYRegister(hmdio, prtad, regad, data);

Introducing stm32 MDIO driver #71012

Introducing stm32 MDIO driver #71012

Conversation

spectrum70 commented Apr 2, 2024 • edited

gramsay0 commented Apr 4, 2024 • edited

spectrum70 commented Apr 4, 2024

gramsay0 commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 commented Apr 9, 2024

gramsay0 commented Apr 9, 2024

spectrum70 commented Apr 9, 2024 • edited

gramsay0 commented Apr 9, 2024

spectrum70 commented Apr 9, 2024

marwaiehm-st commented Apr 30, 2024

spectrum70 commented Apr 30, 2024 • edited

spectrum70 commented May 2, 2024

spectrum70 commented May 3, 2024

erwango commented May 3, 2024

marwaiehm-st commented May 6, 2024

aescolar commented May 7, 2024

spectrum70 commented Apr 2, 2024 •

edited

gramsay0 commented Apr 4, 2024 •

edited

spectrum70 Apr 8, 2024 •

edited

spectrum70 commented Apr 9, 2024 •

edited

spectrum70 commented Apr 30, 2024 •

edited