Re: [PATCH net-next v6] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 5/21/26 04:57, Petr Wozniak wrote:
> The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
> unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
> vendor/part-number combination.  This works for modules that genuinely
> implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
> that share the same EEPROM strings without having such a bridge.
> 
> The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
> media converter with no I2C-to-MDIO bridge.  Its EEPROM reports
> vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
> 00:00:00, making OUI-based differentiation impossible.  With
> MDIO_I2C_ROLLBALL forced, the module silently ACKs the unlock password
> write, the MDIO bus is created, but no PHY responds; the SFP state
> machine cycles through the RollBall PHY-probe retry window before
> reporting no PHY.
> 
> Move the probe into i2c_mii_init_rollball() in mdio-i2c.c, where the
> RollBall protocol constants are already defined.  After sending the
> unlock password, issue a CMD_READ and poll for CMD_DONE up to 200 ms
> (10 x 20 ms, matching the existing rollball poll tolerance).  A genuine
> RollBall bridge asserts CMD_DONE within that window; modules without a
> bridge never do, so i2c_mii_init_rollball() returns -ENODEV.
> mdio_i2c_alloc() propagates -ENODEV to the caller to signal that no
> bridge is present and PHY probing should be skipped.
> sfp_sm_add_mdio_bus() catches -ENODEV and transitions
> sfp->mdio_protocol to MDIO_I2C_NONE so the rest of the state machine
> skips PHY probing for this module.
> 
> Any I2C-level error (NACK, timeout) during the probe is also treated as
> -ENODEV: if the module does not respond at I2C address 0x51 at all,
> there is certainly no RollBall bridge there, and SFP initialization
> should not abort.
> 
> The probe writes are safe with respect to SFP EEPROM integrity: only
> modules explicitly listed in the quirk table enter this path, and the
> RollBall password unlock write to 0x51 was already issued by
> i2c_mii_init_rollball() before the probe for all such modules.  Any
> module without a device at 0x51 NACKs the transfer and is treated as
> -ENODEV.
> 
> Add "OEM"/"SFP-10G-T-I" to the quirk table so RTL8261BE modules enter
> the probe path; genuine RollBall modules continue to work as before.
> 
> Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> 
> Changes since v5 (Sashiko AI review):
>    - Treat I2C NACK/errors in i2c_mii_init_rollball() as -ENODEV so
>      modules without a 0x51 EEPROM do not abort SFP initialization
>    - Replace fixed 70 ms wait with 10 x 20 ms poll (total 200 ms),
>      matching the existing i2c_rollball_mii_poll() tolerance and
>      preventing false -ENODEV on slow RollBall bridges
> 
> Changes since v4 (feedback from Maxime Chevallier):
>    - Fix commit message: replace "stalls" with accurate description of
>      the RollBall PHY-probe retry window
>    - Fix variable declaration order in i2c_mii_probe_rollball() to
>      follow reverse-xmas tree (descending line length)
>    - Remove spurious alignment space on "SFP-10G-T" quirk entry
>    - Document that -ENODEV from mdio_i2c_alloc() means no bridge present,
>      PHY probing should be skipped
> 
> Changes since v3 (feedback from Jakub Kicinski):
>    - Drop spurious Tested-by: tag -- author and tester are the same person
>    - Use PATCH net-next subject prefix
>    - Move -ENODEV handling from sfp_i2c_mdiobus_create() into
>      sfp_sm_add_mdio_bus() so bus-creation code does not mutate
>      sfp->mdio_protocol; the state machine is the correct place for
>      protocol-state transitions
>    - Split combined variable declaration for clarity
> 
> Changes since v2:
>    - Compile-tested and hardware-tested on BPI-R4 (MT7988A, 6.12.87)
>    - RTL8261BE (OEM/SFP-10G-T-I): probes MDIO_I2C_NONE, link Up 10Gbps
>    - Genuine RollBall (OEM/SFP-10G-T): bridge detected, link Up 10Gbps
> 
>   drivers/net/mdio/mdio-i2c.c | 65 ++++++++++++++++++++++++++++++------
>   drivers/net/phy/sfp.c       | 16 ++++++++--
>   2 files changed, 68 insertions(+), 13 deletions(-)
> --- a/drivers/net/mdio/mdio-i2c.c
> +++ b/drivers/net/mdio/mdio-i2c.c
> @@ -352,6 +352,54 @@
>   	return 0;
>   }
>   
> +static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
> +{
> +	u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
> +	u8 cmd_buf[]  = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
> +	u8 cmd_addr   = ROLLBALL_CMD_ADDR;
> +	struct i2c_msg msgs[2];
> +	u8 result;
> +	int ret;
> +	int i;
> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len   = sizeof(data_buf);
> +	msgs[0].buf   = data_buf;
> +	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = 0;
> +	msgs[1].len   = sizeof(cmd_buf);
> +	msgs[1].buf   = cmd_buf;
> +
> +	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return -ENODEV;
> +	if (ret)
> +		return ret;
> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len   = 1;
> +	msgs[0].buf   = &cmd_addr;
> +	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].len   = 1;
> +	msgs[1].buf   = &result;
> +
> +	for (i = 0; i < 10; i++) {
> +		msleep(20);
> +		ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
> +		if (ret < 0)
> +			return -ENODEV;
> +		if (ret)
> +			return ret;

I think this check is not needed, it seems to me that 
i2c_transfer_rollball() always returns either a negative number, or 
zero. All the calls in i2c_transfer_rollball() end-up returning the 
return value from __i2c_transfer_err(), which is always either negative 
or 0.

There's a similar check below that can be simplified as well

Maxime



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy