Driver support for bcm84891 SFP+#192
Open
meehien wants to merge 6 commits into
Open
Conversation
after insertion, returning 0xffff for all reads. get_phy_c45_ids()
detects this (devices-in-package reads as all-ones) and returns
-ENODEV, which the SFP state machine handles with its 25 x 1s retry
loop (R_PHY_RETRY / phy_t_retry set by sfp_fixup_rollball()).
However, some of these modules (observed with an "OEM SFP-10G-T"
containing a Broadcom BCM84891) exit the gated window slightly before
the PHY's identifier registers are stable: devices-in-package reads a
plausible value while MII_PHYSID1/2 in all MMDs still read zero.
get_phy_c45_ids() has no validity check for this case, so a phydev is
created with all-zero c45 device_ids. No PHY driver can match a zero
ID (phy_bus_match() only skips 0xffffffff), so phy->drv is NULL,
phylink_sfp_connect_phy() rejects the PHY, and sfp_add_phy() fails
with -EINVAL:
mtk_soc_eth 15100000.ethernet lsfp: PHY i2c:sfp2:11 (id 0x00000000)
has no driver loaded
sfp sfp2: sfp_add_phy failed: -EINVAL
Unlike -ENODEV, this error path sends the state machine to SFP_S_FAIL,
which is terminal until the module is physically re-seated.
Detect the all-zero identifier case in sfp_sm_probe_phy() and return
-ENODEV so the existing retry logic applies. Also log the c45
identifiers when sfp_add_phy() fails, so a genuinely unsupported PHY
model is diagnosable from dmesg.
command register for ROLLBALL_CMD_DONE, with a budget of 10 x 20 ms. When the poll times out, i2c_mii_read_rollball() returns 0xffff -- indistinguishable from register data. Some modules (observed: "OEM SFP-10G-T" with a Broadcom BCM84891) intermittently take longer than the ~200 ms budget to complete a command, well past the initial 25 s access-denial window. Each timed-out read then injects a spurious 0xffff word into whatever the caller is reading. During PHY probe this corrupts the C45 identifiers (e.g. devs_in_pkg 0xc000ffff, PMA ID 0xffff5081 where the true values are 0xc000009b and 0x35905081), so no PHY driver matches and sfp_add_phy() fails with -EINVAL, leaving the SFP state machine in SFP_S_FAIL until the module is re-seated. The same mechanism can corrupt reads and silently drop writes at runtime after a successful probe. Re-issue the command up to 3 times when the completion poll times out, in both the read and write paths. Re-issuing a read of the same devad/reg is idempotent, so a late completion of a previous attempt still yields data for the correct register. The 0xffff-on-timeout semantic is preserved once retries are exhausted, keeping the existing behaviour for the initial access-denial window (where get_phy_c45_ids() maps all-ones to -ENODEV and the SFP state machine retries the whole probe).
whose identifiers are partially corrupt: a RollBall read that times out returns 0xffff for one 16-bit half of an identifier while the other half reads real data (observed: PMA ID 0xffff5081 for a BCM84891 whose true ID is 0x35905081). Such an identifier matches no driver and still leads to a hard sfp_add_phy() -EINVAL failure. Tighten the check: an identifier is only considered plausible if it is non-zero and neither 16-bit half reads 0xffff. If no MMD carries a plausible identifier, treat the PHY as not ready and return -ENODEV so the state machine's retry logic applies. A legitimate PHY ID with a 16-bit half of exactly 0xffff is not known to exist; if one ever appears, it would only cause probe retries for that module, diagnosable via the retry log message. Depends on: "net: phy: sfp: retry PHY probe when c45 identifiers read as zero"
-ENODEV, reflecting the board-soldered configuration it was written for. The BCM84891 is however also the PHY inside various RollBall "OEM SFP-10G-T" copper SFP+ modules, where phylink selects 10GBASE-R as the host interface (with SGMII/2500BASE-X rate switching), exactly as for the BCM84881. In that configuration a successful PHY probe still fails at attach time: sfp sfp2: PHY c45 ids: PMA 0x35905081 PCS ... devs_in_pkg ... sfp sfp2: sfp_add_phy failed: -ENODEV with the -ENODEV originating from phy_init_hw() -> bcm8489x_config_init(). Accept the same interface trio as the BCM84881 in addition to USXGMII, and populate possible_interfaces with all four so phylink can rate-switch on copper linkdowns. The LPOWER clearing is kept unconditional; it is harmless on module-hosted PHYs.
An OEM SFP-10G-T module with a BCM84891 is observed to stall its I2C mailbox for long stretches while the PHY firmware is busy: under back-to-back register access, the first command after an idle period completes but subsequent commands routinely exceed the 200 ms budget, even with command re-issue retries in place. dmesg shows the probe reading one register successfully and then timing out on ~30 consecutive reads. Extend the budget to 50 iterations (~1 s per attempt). Healthy modules complete in <= 70 ms and are unaffected; the extra budget only spends time when the module is actually stalling, where the alternative is returning fabricated 0xffff data.
LINK_INBAND_DISABLE, but does not implement .config_inband. When a BCM84881-family PHY is used with a phylink MAC in inband mode (e.g. any SFP module PHY, or managed = "in-band-status" in DT), phylink's negotiation resolves to outband operation and sets phy_ib_mode = LINK_INBAND_DISABLE, then calls phy_config_inband() to apply it. With no .config_inband method this returns -EOPNOTSUPP, which phylink_major_config() treats as a fatal configuration error: mtk_soc_eth 15100000.ethernet lsfp: phy_config_inband: -EOPNOTSUPP major_config_failed then forces the link down in phylink_resolve() and holds it down across reconfiguration attempts. The result is a permanently dead link with a perfectly negotiated copper side (observed with a BCM84891-based "OEM SFP-10G-T" module on mt7988: copper AN completes at 10G, carrier never comes up). Implement config_inband: the PHY does not generate inband signalling in any mode, so it is permanently in the "disabled" state and a request to disable inband succeeds as a no-op. Any other request cannot be satisfied and returns -EINVAL (phylink will not request one, given the capabilities this driver reports).
meehien
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check this series because it was developed with Anthropic Fable 5. I had issues getting the module to work until now because the mainstream driver implementation is not complete. The timings for this SFP+ module are also quite bad.
This series of patches registers the module correctly and there are no crashes. As far as I can tell it works perfectly, but I'm no expert in the SFP+ state machine so someone should check properly.