Differential D50128
ixgbe: add MDIO bus support Authored by adrian on May 3 2025, 2:06 AM. Tags None Referenced Files
Subscribers
Details
This works enough to let me see the marvell switch on the MDIO bus. It uses clause 22, which ixgbe's existing MDIO code doesn't currently Since mdio(4) is now required, add it where appropriate to GENERIC kernels.
Diff Detail
Event Timeline
Comment Actions In general this looks fine aside from the cleanups you noted.. if_ix.c can be consider the FreeBSD "driver" where we can host our own changes and that seems the way DPDK structured it too with the functions in the PMD so no particular problem. But for posterity, in general Intel likes to stuff generic hardware dependent stuff in ixgbe_{chip}.c and access it through ixgbe_api.c which would kind of make sense here but we might also consider those files "upstream" (i.e. we can replay DPDK commits or out of tree driver commits more freely to them) so you are following DPDK as "upstream" in this case.
Comment Actions As far as i can tell, DPDK kinda "broke" the c22 routines later on in their development. The chipsets docs for the x550 state the c22 support isn't available, but the x553 backplane docs say it is. I had to go back over their git history to find the initial import. They eventually replaced em with c45 only versions in the PHY code and left the c22 versions in rte_pmd_ixgbe.c. They don't even have an API for doing generic MDIO through ixgbe_api :(
Comment Actions I would be somewhat curious why we need to serialize like this. Note that nothing else prodding the swfw in ixgbe is explicitly locked or asserted. Comment Actions I was thinking about ixgbe_if_init and ixgbe_config_link. IIRC under the hood it does couple of MDIO reads and writes. The SWFW semaphore is hold for individual accesses, but not during whole operation. Without CTX lock writes from mdio bus may interfere with that and bringing link up may fail. TBH now I wonder if we shouldn't hold CTX lock also while changing advertised speeds or flow control settings. Comment Actions ixgbe_if_init and ixgbe_config_link are all be under the CTX lock.
I think actually changing the link happens in the admin task which is protected by CTX lock. So reading or writing with swfw should be enough to serialize the mdio access. A problem would exist if we create some deadlock but I don't see that offhand with what is going on here (but I haven't thought hard about it). Comment Actions @adrian Would you mind posting device id and hw->phy.type from the system you're using for testing? Comment Actions So this panics the kernel if e6000sw is built in or loaded at boot time. From someone who has been testing it: {{{ cpuid = 0 ... the iflib ctx lock is held during the calls to the driver attach/post_attach. We /may/ need to move the child bus attach stuff to a new iflib routine that's documented to being called without the ctx lock held? Comment Actions
I don't know how I missed that while reading the datasheets, but there are two registers used for SW/FW synchronization. IXGBE_SWSM is used to ensure that only one SW thread is accessing HW. I'm sorry for misleading you.
Because @kbowling is right about the semaphore I think we don't need to use a iflib CTX lock for MDIO read operation. For writes I think CTX lock is still required. Some operations on PHY use read/modify/write pattern (e.g. ixgbe_init_ext_t_x550em). Without CTX lock write from MDIO bus may get between that read and write. Regarding E610 - it does have IXGBE_MSCA register, but it's strongly advised to use ACI commands for PHY access instead. It may be better idea to not add independent interface but set phy->ops.read_reg_mdi and phy->ops.write_reg_mdi for X553 backplane and rely on phy.ops.read_reg and phy.ops.write_reg to do the right thing for given HW. Comment Actions ooooo that's good to know. We need a new API for two reasons:
If we can get that cleaned up in the Intel tree then I'm happy cleaning this up too. I'd /much/ prefer to use the PHY ops. Let's get the E610/E810 support landed first, so we can then churn on this? I'm not in a rush, we at least now know how to make this work. :-) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||