drivers: ethernet: xlnx_gem: switch integrated MDIO/PHY support over to Zephyr's framework#87313
drivers: ethernet: xlnx_gem: switch integrated MDIO/PHY support over to Zephyr's framework#87313ibirnbaum wants to merge 11 commits intozephyrproject-rtos:mainfrom
Conversation
zephyrbot
added
area: MDIO
area: Ethernet
platform: Xilinx
zephyrbot
requested review from
decsny,
henrikbrixandersen,
lmajewski,
maass-hamburg,
michalsimek,
pdgendt,
vaishnavachath and
vanti
ibirnbaum
force-pushed
the
xlnx_gem_mdio
branch
3 times, most recently
from
50db3d0 to
cc57b2d
Compare
|
@ibirnbaum I saw that you added two phy drivers, what is the benefit of them, to the already existing generic |
maass-hamburg
left a comment
maass-hamburg
left a comment
There was a problem hiding this comment.
entry in the migration guide needed
ibirnbaum
force-pushed
the
xlnx_gem_mdio
branch
from
cc57b2d to
50872bf
Compare
I can add the interrupt support, however, as mentioned in the PR's description, my initial intention was just to carry over what was already working in the driver's local MDIO/PHY code to Zephyr's respective frameworks and then go for extensions afterwards. If combining such a transitional PR with new features isn't an issue for you, I can add the support now. What's your preference? When it comes to having a separate driver aside from the generic one, there is some selfishness involved. The reason is that Weidmüller uses the DP83822 (and its predecessor, the TLK105) in conjunction with several industrial field buses which use Ethernet as physical layer (e.g. EtherCAT, Ethernet/IP, Profinet, Powerlink, Modbus TCP) in the field bus couplers of the u-remote product range. For EtherCAT, ProfiNet and Ethernet/IP, some settings which go beyond the generic register set have to be altered. Depending on which bus is implemented, those settings relate to LED behavior (MLEDCR register), Analog Power Detect Control (APDS/APDC registers), Auto MDIX, Force MDIX, Robust Auto MDIX (PHYCR/CR1 registers) and Fast Link Down (CR3 register). My intention would be to make those settings available to everyone, likely in the form of DT properties. The old PHY code custom to the GEM driver contained some of those (and they'd be carried over for now with the adoption of this PR), but we so far haven't gone public with everything we have nowadays as a) our exact settings probably won't work for someone else, b) we knew that maintaining the custom GEM MDIO/PHY code would likely be a dead end eventually, and c) the varying configurations per firmware build variant are probably better handled via the DT rather than littering the driver code with ifdefs. The assumption is that those options for this PHY might eventually also be useful to someone else. The afforementioned DP83867 GBit PHY requires a separate driver as it has a specific issue revolving around the bootstrap configuration pins. Certain boards exist, amongst them UltraScale boards made by Xilinx themselves (e.g. ZCU102), where the bootstrap configuration sometimes causes the PHY to come up in self-test mode, which has to be handled in software. In Linux, a custom DT property exists for this issue, IIRC named something like "rx-strap-quirk". I'll check in the existing driver if this is already handled properly. Regarding the Marvell Alaska PHYs, I'll check to what extent they're compatible with the generic approach. Configuration of those PHYs comes with some bank switching and frequent resets, and at least the common Digilent-made Zynq-based eval boards I know of couldn't be used for implementing interrupt support as the IRQ line simply isn't wired up on those boards. If the PHY's configuration approach requires the custom driver, somebody else will have to retrofit the interrupt support. EDIT: the DP83867 driver doesn't seem to handle that issue yet, as the related register CFG4 doesn't seem to be referenced anywhere. |
Noted, I'll take care of it |
If you can get both phys working with just the generic phy driver, its preferred to use that now and and the more specific phys in a later PR, when the extra features can be set and used. |
ibirnbaum
force-pushed
the
xlnx_gem_mdio
branch
from
50872bf to
b944a0f
Compare
drivers/ethernet/eth_xlnx_gem.c
Outdated
| net_if_set_link_addr(iface, dev_data->mac_addr, 6, NET_LINK_ETHERNET); | ||
| ethernet_init(iface); | ||
| net_if_carrier_off(iface); | ||
| net_eth_carrier_off(iface); |
There was a problem hiding this comment.
consider making the phy-handle required in the xlnx,gem.yaml. It seems to be needed to configure the GEM Network Configuration Register and clock speed. also without a phy the carrier would never be set to on.
|
@pdgendt @maass-hamburg |
|
Re-based against main due to the introduction of set_config in a separate PR. Some of the comments have been taken care of already, others are still to follow. Checking that the tests still pass after the rebase. Haven't gotten around to testing the DP83822 and the Alaska PHYs with just the generic PHY support, that's still on my list. |
|
Hi @ibirnbaum Could you please Rebase this PR? The following PR is now merged: |
coming up soon |
|
Thanks for the update, @ibirnbaum. This PR is a dependency for some of our ongoing development work. We’ll wait for the rebased version. |
|
Hi @ibirnbaum, Gentle follow-up on the rebase for this PR. As it’s a dependency for our current work, we’d appreciate any update on progress or if there are blockers we can help with. Thanks! |
Remove the legacy MDIO and PHY support for the GEM MAC which dates back to 2021 and doesn't use the MDIO/PHY frameworks provided by Zephyr nowadays, as those didn't exist back then. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Separate the MDIO functionality from the Xilinx GEM MAC driver into a separate driver conforming with the standard MDIO API. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Enable access to the registers used to set a GEM instance's TX clock frequency. Without this memory mapping, the GEM controllers are only usable without PHY management via MDIO with a fixed TX clock frequency set by the FSBL. If MDIO and PHY management are enabled for either GEM on the Zynq, the system will crash with a data abort without this mapping. The ZynqMP is not affected by this issue, as even with the MPU enabled, the relevant SLCR register space is covered by the "peripherals" mapping entry in the ZynqMP's MPU region table. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Add a new binding for each instance of the Xilinx GEM Ethernet controller, so that separate MAC and MDIO nodes can be specified as child nodes at the same level with no direct hierachical dependency with one another. This is required for the phy-handle reference to work properly. This DT structure is in sync with other Ethernet con- trollers having integrated MDIO facilities, such as those supported for ST and NXP SoCs. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Registers are no longer specified at this level now that the GEM's MAC is a child node below the new GEM Controller parent node, where the register spaces are to be specified instead. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Remove any properties from the Xilinx GEM DT node specification and the associated header in dt-bindings that relate to the legacy MDIO/PHY support. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Remove all instances of DT properties relating to the legacy MDIO/PHY support. Switch over to the new parent/child node DT layout for the GEM. Add the new MDIO nodes for GEM0 and GEM1. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Remove all instances of DT properties relating to the legacy MDIO/PHY support. Add the new MDIO nodes for GEM[0..3]. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
Remove all data/functions/function calls relating to the now removed legacy MDIO/PHY support directly integrated into the GEM device driver. Switch over to using the standard MDIO/PHY interfaces which will now refer to the new separate MDIO driver and the separated PHY drivers. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
enable MDIO in the board's Kconfig, specify the Ethernet operational mode for QEMU, add/configure MDIO/PHY nodes in the device tree. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
ibirnbaum
force-pushed
the
xlnx_gem_mdio
branch
from
9230221 to
dbfa0d6
Compare
Add description of the changes made to the 4.4 migration guide. Signed-off-by: Immo Birnbaum <mail@birnbaum.immo>
zephyrbot
added
the
area: Devicetree Binding
ibirnbaum
force-pushed
the
xlnx_gem_mdio
branch
from
dbfa0d6 to
31b4588
Compare
zephyrbot
requested review from
MaureenHelm,
maass-hamburg,
michalsimek and
stephanosio
|
|
Hi @ibirnbaum DNM label can be removed for this Pull Request; |
This rebase is a dry run so far, as I don't have access to actual hardware for a final test run before Thursday. Also, I'm currently re-basing some more extensions of the driver against this code base. |
|
Hi @michalsimek This PR looks fine to me. Could you please approve; |
ok. FYI; With previous version of pull request i verified on AMD Versal Gen2 R52 hardware. it worked for me; |
Sounds good! I'll take care of the lower end of things, i.e. the Zynq-7000 and the ZynqMP RPU on the ZCU102. By doing so, the tests will cover both an MMU-based as well as an MPU-based system. |
Hi @ibirnbaum I had earlier verified this on the ZynqMP RPU with ZCU102 hardware, and it worked fine. As the core driver functionality like ping is already working, it may be better to proceed with merging this and then continue with additional improvements with new pull request, i also have a plan to add support for 64bit on top of it. |
7 participants
When the device driver for the GEM Ethernet controller was first implemented, Zephyr's framework for MDIO and PHY devices was still in its very early stages and was too limited for use in conjunction with the GEM MAC. While MDIO-less use cases exist for the GEM (such as connecting it to an Ethernet switch in the FPGA part of the chip), pretty much every Zynq-/ZynqMP-based eval board comes with a PHY, and in order to support more flexible use than just supporting the one default speed setting configured by the FSBL at power-up, including the basic ability to detect link up/down, MDIO and PHY support were integrated directly into the driver. The GEM driver was one of two drivers to do it this way, and this internal implementation outlasted the appearance of generic MDIO and PHY support for quite some time.
Now that the limitations of the early stages of the generic MDIO/PHY support within Zephyr no longer exist, I propose switching the GEM's MDIO and PHY capabilities over to the facilities Zephyr provides nowadays. This to me appears beneficial as the addition of a new supported PHY (such as the TI DP83867 which is common in conjunction with the ZynqMP) is not limited to a single Ethernet MAC and also, it's not an addition to code that will inevitably at some point be a dead end if demands are made for the remaining device drivers bypassing the dedicated frameworks to switch over.
Considering that we're talking about two supported SoCs, one in-tree target board definition with a supported (simulated) PHY, the MDIO driver itself, two supported PHYs and a lot of code that has to be shifted around and broken up into pieces as well as obsolete configuration items from the DT that are removed due to them being replaced with those provided by the MDIO/PHY framework, this PR is a pretty big package. However, it contains everything that is required to just keep running what was already running - the integration into the MDIO shell is one little feature that is added on top.
There's no consideration yet if there's settings that could or should be added to the supported PHYs' DTs (a few PHYs have custom DT items, such as the DP83867's "RX strap quirk" setting), or if any setting that is being applied in the PHY drivers' code should be enabled by default as they are right now (e.g. should 'robust auto MDIX' always be on by default in the TI PHY).
This PR would open up the use of the PHYs so far only supported by the GEM to everyone, allowing anyone who is more familiar with those PHYs and thinks that things should be done differently to make suggestions in that direction.