[PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

Hi everyone,

Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
access SFP modules downstream. These controllers are actually SMBus controllers
that can only perform single-byte accesses for read and write.

This series adds support for accessing SFP modules through single-byte SMBus,
which could be relevant for other setups.

The first patch deals with the SFP module access by itself, for addresses 0x50
and 0x51.

The second patch allows accessing embedded PHYs within the module with single-byte
SMBus, adding this in the mdio-i2c driver.

As raw i2c transfers are always more efficient, we make sure that the smbus accesses
are only used if we really have no other choices.

This has been tested with the following modules (as reported upon module insertion)

Fiber modules :

	UBNT             UF-MM-1G         rev      sn FT20051201212    dc 200512
	PROLABS          SFP-1GSXLC-T-C   rev A1   sn PR2109CA1080     dc 220607
	CISCOSOLIDOPTICS CWDM-SFP-1490    rev 1.0  sn SOSC49U0891      dc 181008
	CISCOSOLIDOPTICS CWDM-SFP-1470    rev 1.0  sn SOSC47U1175      dc 190620
	OEM              SFP-10G-SR       rev 02   sn CSSSRIC3174      dc 181201
	FINISAR CORP.    FTLF1217P2BTL-HA rev A    sn PA3A0L6          dc 230716
	OEM              ES8512-3LCD05    rev 10   sn ESC22SX296055    dc 220722
	SOURCEPHOTONICS  SPP10ESRCDFF     rev 10   sn E8G2017450       dc 140715
	CXR              SFP-STM1-MM-850  rev 0000 sn K719017031       dc 200720

 Copper modules

	OEM              SFT-7000-RJ45-AL rev 11.0 sn EB1902240862     dc 190313
	FINISAR CORP.    FCLF8521P2BTL    rev A    sn P1KBAPD          dc 190508
	CHAMPION ONE     1000SFPT         rev -    sn     GBC59750     dc 19110401

DAC :

	OEM              SFP-H10GB-CU1M   rev R    sn CSC200803140115  dc 200827

In all cases, read/write operations happened without errors, and the internal
PHY (if any) was always properly detected and accessible

I haven't tested with any RollBall SFPs though, as I don't have any, and I don't
have Copper modules with anything else than a Marvell 88e1111 inside. The support
for the VSC8552 SMBus may follow at some point.

Thanks,

Maxime

Maxime Chevallier (2):
  net: phy: sfp: Add support for SMBus module access
  net: mdio: mdio-i2c: Add support for single-byte SMBus operations

 drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
 drivers/net/phy/sfp.c       | 65 +++++++++++++++++++++++++++---
 2 files changed, 138 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH net-next 1/2] net: phy: sfp: Add support for SMBus module access

[Maxime Chevallier]

The SFP module's eeprom and internals are accessible through an i2c bus.
However, all the i2c transfers that are performed are SMBus-style
transfers for read and write operations.

It is possible that the SFP might be connected to an SMBus-only
controller, such as the one found in some PHY devices in the VSC85xx
family.

Introduce a set of sfp read/write ops that are going to be used if the
i2c bus is only capable of doing smbus byte accesses.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/sfp.c | 65 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 9369f5297769..65d1fa836db8 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -690,14 +690,69 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 	return ret == ARRAY_SIZE(msgs) ? len : 0;
 }
 
-static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
+static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
+			  size_t len)
 {
-	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
-		return -EINVAL;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	union i2c_smbus_data smbus_data;
+	u8 *data = buf;
+	int ret;
+
+	while (len) {
+		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
+				     I2C_SMBUS_READ, dev_addr,
+				     I2C_SMBUS_BYTE_DATA, &smbus_data);
+		if (ret < 0)
+			return ret;
 
+		*data = smbus_data.byte;
+
+		len--;
+		data++;
+		dev_addr++;
+	}
+
+	return data - (u8 *)buf;
+}
+
+static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
+			   size_t len)
+{
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	union i2c_smbus_data smbus_data;
+	u8 *data = buf;
+	int ret;
+
+	while (len) {
+		smbus_data.byte = *data;
+		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
+				     I2C_SMBUS_WRITE, dev_addr,
+				     I2C_SMBUS_BYTE_DATA, &smbus_data);
+		if (ret)
+			return ret;
+
+		len--;
+		data++;
+		dev_addr++;
+	}
+
+	return 0;
+}
+
+static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
+{
 	sfp->i2c = i2c;
-	sfp->read = sfp_i2c_read;
-	sfp->write = sfp_i2c_write;
+
+	if (i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
+		sfp->read = sfp_i2c_read;
+		sfp->write = sfp_i2c_write;
+	} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		sfp->read = sfp_smbus_read;
+		sfp->write = sfp_smbus_write;
+	} else {
+		sfp->i2c = NULL;
+		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.48.1

[PATCH net-next 2/2] net: mdio: mdio-i2c: Add support for single-byte SMBus operations

[Maxime Chevallier]

PHYs that are within copper SFP modules have their MDIO bus accessible
through address 0x56 (usually) on the i2c bus. The MDIO-I2C bridge is
desgned for 16 bits accesses, but we can also perform 8bits accesses by
reading/writing the high and low bytes sequentially.

This commit adds support for this type of accesses, thus supporting
smbus controllers such as the one in the VSC8552.

This was only tested on Copper SFP modules that embed a Marvell 88e1111
PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-i2c.c b/drivers/net/mdio/mdio-i2c.c
index da2001ea1f99..202f486e71f1 100644
--- a/drivers/net/mdio/mdio-i2c.c
+++ b/drivers/net/mdio/mdio-i2c.c
@@ -106,6 +106,62 @@ static int i2c_mii_write_default_c22(struct mii_bus *bus, int phy_id, int reg,
 	return i2c_mii_write_default_c45(bus, phy_id, -1, reg, val);
 }
 
+static int smbus_byte_mii_read_default_c22(struct mii_bus *bus, int phy_id,
+					   int reg)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	union i2c_smbus_data smbus_data;
+	int val = 0, ret;
+
+	if (!i2c_mii_valid_phy_id(phy_id))
+		return 0;
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_READ, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	val = ((smbus_data.byte & 0xff) << 8);
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_READ, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	val |= (smbus_data.byte & 0xff);
+
+	return val;
+}
+
+static int smbus_byte_mii_write_default_c22(struct mii_bus *bus, int phy_id,
+					    int reg, u16 val)
+{
+	struct i2c_adapter *i2c = bus->priv;
+	union i2c_smbus_data smbus_data;
+	int ret;
+
+	if (!i2c_mii_valid_phy_id(phy_id))
+		return 0;
+
+	smbus_data.byte = ((val & 0xff00) >> 8);
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_WRITE, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0)
+		return ret;
+
+	smbus_data.byte = val & 0xff;
+
+	ret = i2c_smbus_xfer(i2c, i2c_mii_phy_addr(phy_id), 0,
+			     I2C_SMBUS_WRITE, reg,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+
+	return ret < 0 ? ret : 0;
+}
+
 /* RollBall SFPs do not access internal PHY via I2C address 0x56, but
  * instead via address 0x51, when SFP page is set to 0x03 and password to
  * 0xffffffff.
@@ -378,13 +434,26 @@ static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
 		return 0;
 }
 
+static bool mdio_i2c_check_functionality(struct i2c_adapter *i2c,
+					 enum mdio_i2c_proto protocol)
+{
+	if (i2c_check_functionality(i2c, I2C_FUNC_I2C))
+		return true;
+
+	if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) &&
+	    protocol == MDIO_I2C_MARVELL_C22)
+		return true;
+
+	return false;
+}
+
 struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
 			       enum mdio_i2c_proto protocol)
 {
 	struct mii_bus *mii;
 	int ret;
 
-	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
+	if (!mdio_i2c_check_functionality(i2c, protocol))
 		return ERR_PTR(-EINVAL);
 
 	mii = mdiobus_alloc();
@@ -395,6 +464,14 @@ struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
 	mii->parent = parent;
 	mii->priv = i2c;
 
+	/* Only use SMBus if we have no other choice */
+	if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) &&
+	    !i2c_check_functionality(i2c, I2C_FUNC_I2C)) {
+		mii->read = smbus_byte_mii_read_default_c22;
+		mii->write = smbus_byte_mii_write_default_c22;
+		return mii;
+	}
+
 	switch (protocol) {
 	case MDIO_I2C_ROLLBALL:
 		ret = i2c_mii_init_rollball(i2c);
-- 
2.48.1

Re: [PATCH net-next 2/2] net: mdio: mdio-i2c: Add support for single-byte SMBus operations

[Andrew Lunn]

> This was only tested on Copper SFP modules that embed a Marvell 88e1111
> PHY.

Does the Marvell PHY datasheet say what happens when you perform 8 bit
accesses to 16 bit registers, such at the BMSR?

	Andrew

Re: [PATCH net-next 2/2] net: mdio: mdio-i2c: Add support for single-byte SMBus operations

[Maxime Chevallier]

On Mon, 24 Feb 2025 04:36:49 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > This was only tested on Copper SFP modules that embed a Marvell 88e1111
> > PHY.  
> 
> Does the Marvell PHY datasheet say what happens when you perform 8 bit
> accesses to 16 bit registers, such at the BMSR?

It doesn't specifically say what happens to BMSR, however the section
about "how to perform a random read" gives an example of a random
register read that is made of 2 single-byte reads, including the STOP
bit being set in-between reading the upper byte and the lower byte.

While this doesn't exactly specify the BMSR's latching behaviour, it
looks to me that this is a coherent way of reading a register state,
and BMSR's link status register *should* latch until the lower byte is
read.

I'll try it out with one of my modules to make sure though.

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:
> Hi everyone,
> 
> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> access SFP modules downstream. These controllers are actually SMBus controllers
> that can only perform single-byte accesses for read and write.

This goes against SFF-8472, and likely breaks atomic access to 16-bit
PHY registers.

For the former, I quote from SFF-8472:

"To guarantee coherency of the diagnostic monitoring data, the host is
required to retrieve any multi-byte fields from the diagnostic
monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
Power LSB - byte 105 in A2h) by the use of a single two-byte read
sequence across the 2-wire interface."

So, if using a SMBus controller, I think we should at the very least
disable exporting the hwmon parameters as these become non-atomic
reads.

Whether PHY access works correctly or not is probably module specific.
E.g. reading the MII_BMSR register may not return latched link status
because the reads of the high and low bytes may be interpreted as two
seperate distinct accesses.

In an ideal world, I'd prefer to say no to hardware designs like this,
but unfortunately, hardware designers don't know these details of the
protocol, and all they see is "two wire, oh SMBus will do".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

Hi Russell

On Sun, 23 Feb 2025 17:40:37 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:
> > Hi everyone,
> > 
> > Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> > access SFP modules downstream. These controllers are actually SMBus controllers
> > that can only perform single-byte accesses for read and write.  
> 
> This goes against SFF-8472, and likely breaks atomic access to 16-bit
> PHY registers.
> 
> For the former, I quote from SFF-8472:
> 
> "To guarantee coherency of the diagnostic monitoring data, the host is
> required to retrieve any multi-byte fields from the diagnostic
> monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> Power LSB - byte 105 in A2h) by the use of a single two-byte read
> sequence across the 2-wire interface."
> 
> So, if using a SMBus controller, I think we should at the very least
> disable exporting the hwmon parameters as these become non-atomic
> reads.

That makes sense to me, it's best-effort at that point :)

> Whether PHY access works correctly or not is probably module specific.
> E.g. reading the MII_BMSR register may not return latched link status
> because the reads of the high and low bytes may be interpreted as two
> seperate distinct accesses.
> 
> In an ideal world, I'd prefer to say no to hardware designs like this,
> but unfortunately, hardware designers don't know these details of the
> protocol, and all they see is "two wire, oh SMBus will do".

On that particular PHY I'm mentionning, the feature really is advertised
to HW designers as "you connect that TWI to your SFP cage and you're
good". I was very surprised as well when digging deeper and figuring
that not only it's SMBus, and worse, 1-byte accesses only... However
there are already some HW out there with this feature in use. If this
is OK for you to accept that as a "best effort, degraded mode as
there's no hwmon", that's already great :)

Thanks for being so quick !

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Bjørn Mork]

"Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:
>> Hi everyone,
>> 
>> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
>> access SFP modules downstream. These controllers are actually SMBus controllers
>> that can only perform single-byte accesses for read and write.
>
> This goes against SFF-8472, and likely breaks atomic access to 16-bit
> PHY registers.
>
> For the former, I quote from SFF-8472:
>
> "To guarantee coherency of the diagnostic monitoring data, the host is
> required to retrieve any multi-byte fields from the diagnostic
> monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> Power LSB - byte 105 in A2h) by the use of a single two-byte read
> sequence across the 2-wire interface."
>
> So, if using a SMBus controller, I think we should at the very least
> disable exporting the hwmon parameters as these become non-atomic
> reads.

Would SMBus word reads be an alternative for hwmon, if the SMBus
controller support those?  Should qualify as "a single two-byte read
sequence across the 2-wire interface."

> Whether PHY access works correctly or not is probably module specific.
> E.g. reading the MII_BMSR register may not return latched link status
> because the reads of the high and low bytes may be interpreted as two
> seperate distinct accesses.

Bear with me.  Trying to learn here.  AFAIU, we only have a defacto
specification of the clause 22 phy interface over i2c, based on the
88E1111 implementation.  As Maxime pointed out, this explicitly allows
two sequential distinct byte transactions to read or write the 16bit
registers. See figures 27 and 30 in
https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-alaska-88e1111-datasheet.pdf

Looks like the latch timing restrictions are missing, but I still do not
think that's enough reason to disallow access to phys over SMBus.  If
this is all the interface specification we have?

I have been digging around for the RollBall protocol spec, but Google
isn't very helpful.  This list and the mdio-i2c.c implementation is all
that comes up.  It does use 4 and 6 byte transactions which will be
difficult to emulate on SMBus.  But the

	/* By experiment it takes up to 70 ms to access a register for these
	 * SFPs. Sleep 20ms between iterations and try 10 times.
	 */

comment in i2c_rollball_mii_poll() indicates that it isn't very timing
sensitive at all. The RollBall SFP+ I have ("FS", "SFP-10G-T") is faster
than the comment indicates, but still leaves plenty of time for the
single byte SMBus transactions to complete.

Haven't found any formal specification of i2c clause 45 access either.
But some SFP+ vendors have been nice enough to document their protocol
in datasheets.  Examples:

https://www.repotec.com/download/managed-ethernet/ds-ml/01-MOD-M10GTP-DS-verB.pdf
https://www.apacoe.com.tw/files/ASFPT-TNBT-X-NA%20V1.4.pdf

They all seem to agree that 2/4/6 byte accesses are required, and they
offer no single byte alternative even if the presence of a "smart"
bridge should allow intelligent latching.  So this might be
"impossible" (aka "hard") to do over SMBus.   I have no such SFP+ so I
cannot even try.

> In an ideal world, I'd prefer to say no to hardware designs like this,
> but unfortunately, hardware designers don't know these details of the
> protocol, and all they see is "two wire, oh SMBus will do".

I believe you are reading more into the spec than what's actually there.

SFF-8419 defines the interface as

 "The SFP+ management interface is a two-wire interface, similar to
  I2C."

There is no i2c requirement. This does not rule out SMBus. Maybe I am
reading too much into it as well, but in my view "similar to I2C" sounds
like they wanted to include SMBus.

Both the adhoc phy additions and the diagnostic parts of SFF-8472
silently ignores this.  I do not think the blame for any incompatibilty
is solely on the host side here.


Bjørn

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

Hi,

On Tue, 25 Feb 2025 13:38:30 +0100
Bjørn Mork <bjorn@mork.no> wrote:

> "Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> > On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:  
> >> Hi everyone,
> >> 
> >> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> >> access SFP modules downstream. These controllers are actually SMBus controllers
> >> that can only perform single-byte accesses for read and write.  
> >
> > This goes against SFF-8472, and likely breaks atomic access to 16-bit
> > PHY registers.
> >
> > For the former, I quote from SFF-8472:
> >
> > "To guarantee coherency of the diagnostic monitoring data, the host is
> > required to retrieve any multi-byte fields from the diagnostic
> > monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> > Power LSB - byte 105 in A2h) by the use of a single two-byte read
> > sequence across the 2-wire interface."
> >
> > So, if using a SMBus controller, I think we should at the very least
> > disable exporting the hwmon parameters as these become non-atomic
> > reads.  
> 
> Would SMBus word reads be an alternative for hwmon, if the SMBus
> controller support those?  Should qualify as "a single two-byte read
> sequence across the 2-wire interface."

There are different flavors when it comes to what an SMBus controller
can do. In the case of what this patchset supports, its really about
SMBus controllers that can only perform single-byte operations, which
will cause issues here.

What I have is a controller that only supports I2C_FUNC_SMBUS_BYTE, in
that the controller will issue a STOP after reading/writing one byte.

But if you have a controller that supports, say,
I2C_FUNC_SMBUS_WORD_DATA (i.e. 16 bits words xfers), that's already a
different story, as the diags situation Russell mentions will fit in a
word. That will also make MDIO accesses to embedded PHYs easier, at
least for C22. 

> > Whether PHY access works correctly or not is probably module specific.
> > E.g. reading the MII_BMSR register may not return latched link status
> > because the reads of the high and low bytes may be interpreted as two
> > seperate distinct accesses.  
> 
> Bear with me.  Trying to learn here.  AFAIU, we only have a defacto
> specification of the clause 22 phy interface over i2c, based on the
> 88E1111 implementation.  As Maxime pointed out, this explicitly allows
> two sequential distinct byte transactions to read or write the 16bit
> registers. See figures 27 and 30 in
> https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-alaska-88e1111-datasheet.pdf
> 
> Looks like the latch timing restrictions are missing, but I still do not
> think that's enough reason to disallow access to phys over SMBus.  If
> this is all the interface specification we have?
> 
> I have been digging around for the RollBall protocol spec, but Google
> isn't very helpful.  This list and the mdio-i2c.c implementation is all
> that comes up.  It does use 4 and 6 byte transactions which will be
> difficult to emulate on SMBus.  But the
> 
> 	/* By experiment it takes up to 70 ms to access a register for these
> 	 * SFPs. Sleep 20ms between iterations and try 10 times.
> 	 */
> 
> comment in i2c_rollball_mii_poll() indicates that it isn't very timing
> sensitive at all. The RollBall SFP+ I have ("FS", "SFP-10G-T") is faster
> than the comment indicates, but still leaves plenty of time for the
> single byte SMBus transactions to complete.
> 
> Haven't found any formal specification of i2c clause 45 access either.
> But some SFP+ vendors have been nice enough to document their protocol
> in datasheets.  Examples:
> 
> https://www.repotec.com/download/managed-ethernet/ds-ml/01-MOD-M10GTP-DS-verB.pdf
> https://www.apacoe.com.tw/files/ASFPT-TNBT-X-NA%20V1.4.pdf
> 
> They all seem to agree that 2/4/6 byte accesses are required, and they
> offer no single byte alternative even if the presence of a "smart"
> bridge should allow intelligent latching.  So this might be
> "impossible" (aka "hard") to do over SMBus.   I have no such SFP+ so I
> cannot even try.
> 
> > In an ideal world, I'd prefer to say no to hardware designs like this,
> > but unfortunately, hardware designers don't know these details of the
> > protocol, and all they see is "two wire, oh SMBus will do".  
> 
> I believe you are reading more into the spec than what's actually there.
> 
> SFF-8419 defines the interface as
> 
>  "The SFP+ management interface is a two-wire interface, similar to
>   I2C."
> 
> There is no i2c requirement. This does not rule out SMBus. Maybe I am
> reading too much into it as well, but in my view "similar to I2C" sounds
> like they wanted to include SMBus.
> 
> Both the adhoc phy additions and the diagnostic parts of SFF-8472
> silently ignores this.  I do not think the blame for any incompatibilty
> is solely on the host side here.

At least for this series, SMBus by itself isn't the main issue, the
main problem is single-byte SMBus. As soon as you can do 16 bits xfers,
that rules-out a whole class of potential problems (which doesn't mean
there will be no issue :) ), but I haven't really digged into how C45
and rollball will behave in that case.

Note that the series actually doesn't support I2C_FUNC_SMBUS_WORD_DATA,
but doesn't prevent it either :)

The driver for the SMBus controller in the rtl930x SoC you mention seem
to support I2C_FUNC_SMBUS_WORD_DATA and even longer xfers, in which
case you could add SFP support for smbus-only controllers in a much
more reliable way. It could be conceivable to remove the hwmon-disabled
restriction for I2C_FUNC_SMBUS_WORD_DATA.

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Andrew Lunn]

> > Would SMBus word reads be an alternative for hwmon, if the SMBus
> > controller support those?  Should qualify as "a single two-byte read
> > sequence across the 2-wire interface."
> 
> There are different flavors when it comes to what an SMBus controller
> can do. In the case of what this patchset supports, its really about
> SMBus controllers that can only perform single-byte operations, which
> will cause issues here.
> 
> What I have is a controller that only supports I2C_FUNC_SMBUS_BYTE, in
> that the controller will issue a STOP after reading/writing one byte.
> 
> But if you have a controller that supports, say,
> I2C_FUNC_SMBUS_WORD_DATA (i.e. 16 bits words xfers), that's already a
> different story, as the diags situation Russell mentions will fit in a
> word. That will also make MDIO accesses to embedded PHYs easier, at
> least for C22. 

Agreed.

> > > Whether PHY access works correctly or not is probably module specific.
> > > E.g. reading the MII_BMSR register may not return latched link status
> > > because the reads of the high and low bytes may be interpreted as two
> > > seperate distinct accesses.  
> > 
> > Bear with me.  Trying to learn here.  AFAIU, we only have a defacto
> > specification of the clause 22 phy interface over i2c, based on the
> > 88E1111 implementation.  As Maxime pointed out, this explicitly allows
> > two sequential distinct byte transactions to read or write the 16bit
> > registers. See figures 27 and 30 in
> > https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-alaska-88e1111-datasheet.pdf
> > 
> > Looks like the latch timing restrictions are missing, but I still do not
> > think that's enough reason to disallow access to phys over SMBus.  If
> > this is all the interface specification we have?

We are not suggesting it is disallowed. We are simply saying add a
warning that it is possibly broken, bad things can happen, and there
is nothing we can do about it, so don't report issues when it does
break.

It might work enough that users are happy to take the risk, and need
to unplug/plug the cable every so often to get the link back when the
link peer changes etc.

	Andrew

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Tue, Feb 25, 2025 at 01:38:30PM +0100, Bjørn Mork wrote:
> "Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> > On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:
> >> Hi everyone,
> >> 
> >> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> >> access SFP modules downstream. These controllers are actually SMBus controllers
> >> that can only perform single-byte accesses for read and write.
> >
> > This goes against SFF-8472, and likely breaks atomic access to 16-bit
> > PHY registers.
> >
> > For the former, I quote from SFF-8472:
> >
> > "To guarantee coherency of the diagnostic monitoring data, the host is
> > required to retrieve any multi-byte fields from the diagnostic
> > monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> > Power LSB - byte 105 in A2h) by the use of a single two-byte read
> > sequence across the 2-wire interface."
> >
> > So, if using a SMBus controller, I think we should at the very least
> > disable exporting the hwmon parameters as these become non-atomic
> > reads.
> 
> Would SMBus word reads be an alternative for hwmon, if the SMBus
> controller support those?  Should qualify as "a single two-byte read
> sequence across the 2-wire interface."

Looking at the SNIA documents, this should work, so if a SMBus supports
two-byte reads, then we should be able to support hwmon with such a
controller. However, if only single-byte reads are supported, then I
think we should disable hwmon as we'd be going against the statement
I provided above from the docs.

> > Whether PHY access works correctly or not is probably module specific.
> > E.g. reading the MII_BMSR register may not return latched link status
> > because the reads of the high and low bytes may be interpreted as two
> > seperate distinct accesses.
> 
> Bear with me.  Trying to learn here.  AFAIU, we only have a defacto
> specification of the clause 22 phy interface over i2c, based on the
> 88E1111 implementation.  As Maxime pointed out, this explicitly allows
> two sequential distinct byte transactions to read or write the 16bit
> registers. See figures 27 and 30 in
> https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-alaska-88e1111-datasheet.pdf

However, note that it's:

 START ADDR(W) REG RE-START ADDR(R) HIGH STOP
 START ADDR(W) REG RE-START ADDR(R) LOW STOP

and not:

 START ADDR(W) REG STOP START ADDR(R) HIGH STOP
 ...

I forget whether SMBus can do re-starts.

> Looks like the latch timing restrictions are missing, but I still do not
> think that's enough reason to disallow access to phys over SMBus.  If
> this is all the interface specification we have?

I'm not sure what you're referring to here with "latch timing
restrictions".

Reading more of the 88E1111, if one only transfers an odd number of
bytes for a register, it looks like that could cause problems. It
doesn't appear to specify what happens if it receives a write e.g.
for upper byte of register 0, and then receives a write for
register 4 (as a hypothetical example - caused where the lower byte of
register 0 transfer failed.)

We do need to think about these corner cases!

> I have been digging around for the RollBall protocol spec, but Google
> isn't very helpful.  This list and the mdio-i2c.c implementation is all
> that comes up.  It does use 4 and 6 byte transactions which will be
> difficult to emulate on SMBus.  But the
> 
> 	/* By experiment it takes up to 70 ms to access a register for these
> 	 * SFPs. Sleep 20ms between iterations and try 10 times.
> 	 */
> 
> comment in i2c_rollball_mii_poll() indicates that it isn't very timing
> sensitive at all. The RollBall SFP+ I have ("FS", "SFP-10G-T") is faster
> than the comment indicates, but still leaves plenty of time for the
> single byte SMBus transactions to complete.

The "RollBall" protocol isn't official afaics. It got called that
because of the first module that Marek happened to have that implemented
it. Having taken several of these modules apart, it's implemented by a
microcontroller, which may be an Arm Cortex based controller, or might
be 8051 based. Whether the firmware implementation at the high-level
code is the same or not is anyone's guess. We just don't know. It
could be entirely different implementations - and that's the problem.
We already know that the timings for this protocol vary depending on
the module (possibly because of differing implementations) which brings
questions up about the behaviour in obscure cases like single-byte
reads.

> Haven't found any formal specification of i2c clause 45 access either.
> But some SFP+ vendors have been nice enough to document their protocol
> in datasheets.  Examples:
> 
> https://www.repotec.com/download/managed-ethernet/ds-ml/01-MOD-M10GTP-DS-verB.pdf
> https://www.apacoe.com.tw/files/ASFPT-TNBT-X-NA%20V1.4.pdf
> 
> They all seem to agree that 2/4/6 byte accesses are required, and they
> offer no single byte alternative even if the presence of a "smart"
> bridge should allow intelligent latching.  So this might be
> "impossible" (aka "hard") to do over SMBus.   I have no such SFP+ so I
> cannot even try.

Indeed. So maybe we need a stronger kernel message to basically blame
the hardware designer and refuse to operate with modules that _might_
use the Rollball protocol. (Note, it's "might" because if we can't
access the PHY, we don't know for certain that this module is a
copper module.)

> > In an ideal world, I'd prefer to say no to hardware designs like this,
> > but unfortunately, hardware designers don't know these details of the
> > protocol, and all they see is "two wire, oh SMBus will do".
> 
> I believe you are reading more into the spec than what's actually there.

So I'm making up the quote above from SFF-8472.  Okay, if that's where
this discussion is going, I'm done here.

NAK to this patch set.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Bjørn Mork]

"Russell King (Oracle)" <linux@armlinux.org.uk> writes:

>> I believe you are reading more into the spec than what's actually there.
>
> So I'm making up the quote above from SFF-8472.  Okay, if that's where
> this discussion is going, I'm done here.

No, not at all.  That was not what I meant.  Please accept my apologies.
This came out wrong. You are absolutely correct about reading the 16bit
diagnostic registers you quoted. I would never doubt that. I have an
extreme respect for you and your knowledge of these standards and the
practical hardware implications.

It was the conclusion that this fact prevents SMBus hosts I wanted to
question.  I still don't see that.  Some SMBus hosts might be able do 2
byte reads.  And if they can't, then I believe they can safely ignore
these registers without being out of spec.  Like the proposed solution.

I'll shut up now, to avoid confusing the discussion of Maxime's patches
further.


Bjørn

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Tue, Feb 25, 2025 at 07:07:41PM +0100, Bjørn Mork wrote:
> "Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> 
> >> I believe you are reading more into the spec than what's actually there.
> >
> > So I'm making up the quote above from SFF-8472.  Okay, if that's where
> > this discussion is going, I'm done here.
> 
> No, not at all.  That was not what I meant.  Please accept my apologies.
> This came out wrong. You are absolutely correct about reading the 16bit
> diagnostic registers you quoted. I would never doubt that. I have an
> extreme respect for you and your knowledge of these standards and the
> practical hardware implications.
> 
> It was the conclusion that this fact prevents SMBus hosts I wanted to
> question.  I still don't see that.  Some SMBus hosts might be able do 2
> byte reads.  And if they can't, then I believe they can safely ignore
> these registers without being out of spec.  Like the proposed solution.

It doesn't prevent SMBus hosts, but it does prevent SMBus hosts from
being able to be used *reliably* with the diagnostics "EEPROM" to read
the values in a coherent manner.

It also prevents being able to identify the Nokia 3FE46541AA module,
because the module's I2C locks the bus when a single-byte read of
offset 0x51 in the "identity" EEPROM at 0xA0/0x50.

We do already have single-byte mode in the SFP driver, which is
necessary to work around the broken I2C interface on RTL8672 and
RTL9601C which emulate that EEPROM.

There's a lot of "brokenness" out there, and what I've learnt from
dealing with SFPs over the last 10+ years is to be cautious about
*everything*. One thing that can be guaranteed is that a module will
be broken in unexpected ways, and using a different behaviour for
working modules will turn out to break.

The Nokia 3FE46541AA is a brilliant example of a module that
emulates all accesses and fails with single-byte reads, thus making
it incompatible with a SMBus that can only read single bytes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Bjørn Mork]

Maxime Chevallier <maxime.chevallier@bootlin.com> writes:

> Hi everyone,
>
> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> access SFP modules downstream. These controllers are actually SMBus controllers
> that can only perform single-byte accesses for read and write.
>
> This series adds support for accessing SFP modules through single-byte SMBus,
> which could be relevant for other setups.
>
> The first patch deals with the SFP module access by itself, for addresses 0x50
> and 0x51.
>
> The second patch allows accessing embedded PHYs within the module with single-byte
> SMBus, adding this in the mdio-i2c driver.
>
> As raw i2c transfers are always more efficient, we make sure that the smbus accesses
> are only used if we really have no other choices.
>
> This has been tested with the following modules (as reported upon module insertion)
>
> Fiber modules :
>
> 	UBNT             UF-MM-1G         rev      sn FT20051201212    dc 200512
> 	PROLABS          SFP-1GSXLC-T-C   rev A1   sn PR2109CA1080     dc 220607
> 	CISCOSOLIDOPTICS CWDM-SFP-1490    rev 1.0  sn SOSC49U0891      dc 181008
> 	CISCOSOLIDOPTICS CWDM-SFP-1470    rev 1.0  sn SOSC47U1175      dc 190620
> 	OEM              SFP-10G-SR       rev 02   sn CSSSRIC3174      dc 181201
> 	FINISAR CORP.    FTLF1217P2BTL-HA rev A    sn PA3A0L6          dc 230716
> 	OEM              ES8512-3LCD05    rev 10   sn ESC22SX296055    dc 220722
> 	SOURCEPHOTONICS  SPP10ESRCDFF     rev 10   sn E8G2017450       dc 140715
> 	CXR              SFP-STM1-MM-850  rev 0000 sn K719017031       dc 200720
>
>  Copper modules
>
> 	OEM              SFT-7000-RJ45-AL rev 11.0 sn EB1902240862     dc 190313
> 	FINISAR CORP.    FCLF8521P2BTL    rev A    sn P1KBAPD          dc 190508
> 	CHAMPION ONE     1000SFPT         rev -    sn     GBC59750     dc 19110401
>
> DAC :
>
> 	OEM              SFP-H10GB-CU1M   rev R    sn CSC200803140115  dc 200827
>
> In all cases, read/write operations happened without errors, and the internal
> PHY (if any) was always properly detected and accessible
>
> I haven't tested with any RollBall SFPs though, as I don't have any, and I don't
> have Copper modules with anything else than a Marvell 88e1111 inside. The support
> for the VSC8552 SMBus may follow at some point.
>
> Thanks,
>
> Maxime
>
> Maxime Chevallier (2):
>   net: phy: sfp: Add support for SMBus module access
>   net: mdio: mdio-i2c: Add support for single-byte SMBus operations
>
>  drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
>  drivers/net/phy/sfp.c       | 65 +++++++++++++++++++++++++++---
>  2 files changed, 138 insertions(+), 6 deletions(-)

Nice!  Don't know if you're aware, but OpenWrt have had patches for
SMBus access to SFPs for some time:

https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/714-net-phy-sfp-add-support-for-SMBus.patch
https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/712-net-phy-add-an-MDIO-SMBus-library.patch

The reason they carry these is that they support Realtek rtl930x based
switches.  The rtl930x SoCs include an 8 channel SMBus host which is
typically connected to any SFP+ slots on the switch.

There has been work going on for a while to bring the support for these
SoCs to mainline, and the SMBus host driver is already here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-rtl9300.c?id=c366be720235301fdadf67e6f1ea6ff32669c074

I assume DSA and ethernet eventually will follow, making SMBus SFP
support necessary for this platform too.

So thanks for doing this!

FWIW, I don't think the OpenWrt mdio patch works at all.  I've recently
been playing with an 8 SFP+ port switch based on rtl9303, and have tried
to fixup both the clause 22 support and add RollBall and clause 45.
This is still a somewhat untested hack, and I was not planning on
presenting it here as such, but since this discussion is open:
https://github.com/openwrt/openwrt/pull/17950/commits/c40387104af62a065797bc3e23dfb9f36e03851b

Sorry for the format.  This is a patch for the patch already present in
OpenWrt. Let me know if you want me to post the complete patched
mdio-smbus.c for easier reading.

The main point I wanted to make is that we also need RollBall and clause
45 over SMBus.  Maybe not today, but at some point.  Ideally, the code
should be shared with the i2c implementation, but I found that very hard
to do as it is.

As for Russel's comments regarding atomic reads, I'm hoping for the
pragmatic approach and allow all possible features over SMBus. It's not
like we have the option of using i2c on a host which only supports
SMBus.  My experience is that both hwmon and phy access works pretty
well with SMBus byte accesses.

Some examples:

root@s508cl:~# ethtool -m lan1
        Identifier                                : 0x03 (SFP)
        Extended identifier                       : 0x04 (GBIC/SFP defined by 2-wire interface ID)
        Connector                                 : 0x07 (LC)
        Transceiver codes                         : 0x10 0x00 0x00 0x00 0x00 0x00 0x06 0x00 0x00
        Transceiver type                          : 10G Ethernet: 10G Base-SR
        Transceiver type                          : FC: Multimode, 50um (M5)
        Encoding                                  : 0x03 (NRZ)
        BR, Nominal                               : 10300MBd
        Rate identifier                           : 0x00 (unspecified)
        Length (SMF,km)                           : 0km
        Length (SMF)                              : 0m
        Length (50um)                             : 300m
        Length (62.5um)                           : 300m
        Length (Copper)                           : 0m
        Length (OM3)                              : 300m
        Laser wavelength                          : 850nm
        Vendor name                               : OEM
        Vendor OUI                                : 00:00:00
        Vendor PN                                 : SFP-10G-SR
        Vendor rev                                : B
        Option values                             : 0x00 0x1a
        Option                                    : RX_LOS implemented
        Option                                    : TX_FAULT implemented
        Option                                    : TX_DISABLE implemented
        BR margin, max                            : 0%
        BR margin, min                            : 0%
        Vendor SN                                 : 202412240025
        Date code                                 : 241224
        Optical diagnostics support               : Yes
        Laser bias current                        : 7.146 mA
        Laser output power                        : 0.4005 mW / -3.97 dBm
        Receiver signal average optical power     : 0.5088 mW / -2.93 dBm
        Module temperature                        : 52.13 degrees C / 125.83 degrees F
        Module voltage                            : 3.2644 V
        Alarm/warning flags implemented           : Yes
        Laser bias current high alarm             : Off
        Laser bias current low alarm              : Off
        Laser bias current high warning           : Off
        Laser bias current low warning            : Off
        Laser output power high alarm             : Off
        Laser output power low alarm              : Off
        Laser output power high warning           : Off
        Laser output power low warning            : Off
        Module temperature high alarm             : Off
        Module temperature low alarm              : Off
        Module temperature high warning           : Off
        Module temperature low warning            : Off
        Module voltage high alarm                 : Off
        Module voltage low alarm                  : Off
        Module voltage high warning               : Off
        Module voltage low warning                : Off
        Laser rx power high alarm                 : Off
        Laser rx power low alarm                  : Off
        Laser rx power high warning               : Off
        Laser rx power low warning                : Off
        Laser bias current high alarm threshold   : 12.000 mA
        Laser bias current low alarm threshold    : 1.000 mA
        Laser bias current high warning threshold : 10.000 mA
        Laser bias current low warning threshold  : 2.000 mA
        Laser output power high alarm threshold   : 1.5849 mW / 2.00 dBm
        Laser output power low alarm threshold    : 0.1000 mW / -10.00 dBm
        Laser output power high warning threshold : 1.2589 mW / 1.00 dBm
        Laser output power low warning threshold  : 0.1259 mW / -9.00 dBm
        Module temperature high alarm threshold   : 85.00 degrees C / 185.00 degrees F
        Module temperature low alarm threshold    : -10.00 degrees C / 14.00 degrees F
        Module temperature high warning threshold : 80.00 degrees C / 176.00 degrees F
        Module temperature low warning threshold  : -5.00 degrees C / 23.00 degrees F
        Module voltage high alarm threshold       : 3.7000 V
        Module voltage low alarm threshold        : 2.9000 V
        Module voltage high warning threshold     : 3.6000 V
        Module voltage low warning threshold      : 3.0000 V
        Laser rx power high alarm threshold       : 1.9953 mW / 3.00 dBm
        Laser rx power low alarm threshold        : 0.0398 mW / -14.00 dBm
        Laser rx power high warning threshold     : 1.5849 mW / 2.00 dBm
        Laser rx power low warning threshold      : 0.0501 mW / -13.00 dBm

root@s508cl:~# ethtool -m lan3
        Identifier                                : 0x03 (SFP)
        Extended identifier                       : 0x04 (GBIC/SFP defined by 2-wire interface ID)
        Connector                                 : 0x07 (LC)
        Transceiver codes                         : 0x10 0x00 0x00 0x00 0x40 0x00 0x0c 0x00 0x00
        Transceiver type                          : 10G Ethernet: 10G Base-SR
        Transceiver type                          : FC: short distance (S)
        Transceiver type                          : FC: Multimode, 62.5um (M6)
        Transceiver type                          : FC: Multimode, 50um (M5)
        Encoding                                  : 0x06 (64B/66B)
        BR, Nominal                               : 10300MBd
        Rate identifier                           : 0x00 (unspecified)
        Length (SMF,km)                           : 0km
        Length (SMF)                              : 0m
        Length (50um)                             : 30m
        Length (62.5um)                           : 10m
        Length (Copper)                           : 0m
        Length (OM3)                              : 0m
        Laser wavelength                          : 850nm
        Vendor name                               : FS
        Vendor OUI                                : 00:00:00
        Vendor PN                                 : SFP-10G-T
        Vendor rev                                : 
        Option values                             : 0x00 0x1a
        Option                                    : RX_LOS implemented
        Option                                    : TX_FAULT implemented
        Option                                    : TX_DISABLE implemented
        BR margin, max                            : 10%
        BR margin, min                            : 88%
        Vendor SN                                 : F2220644072
        Date code                                 : 220824
        Optical diagnostics support               : Yes
        Laser bias current                        : 6.000 mA
        Laser output power                        : 0.5000 mW / -3.01 dBm
        Receiver signal average optical power     : 0.0000 mW / -inf dBm
        Module temperature                        : 54.22 degrees C / 129.59 degrees F
        Module voltage                            : 3.3368 V
        Alarm/warning flags implemented           : Yes
        Laser bias current high alarm             : Off
        Laser bias current low alarm              : Off
        Laser bias current high warning           : Off
        Laser bias current low warning            : Off
        Laser output power high alarm             : Off
        Laser output power low alarm              : Off
        Laser output power high warning           : Off
        Laser output power low warning            : Off
        Module temperature high alarm             : Off
        Module temperature low alarm              : Off
        Module temperature high warning           : Off
        Module temperature low warning            : Off
        Module voltage high alarm                 : Off
        Module voltage low alarm                  : Off
        Module voltage high warning               : Off
        Module voltage low warning                : Off
        Laser rx power high alarm                 : Off
        Laser rx power low alarm                  : On
        Laser rx power high warning               : Off
        Laser rx power low warning                : On
        Laser bias current high alarm threshold   : 15.000 mA
        Laser bias current low alarm threshold    : 1.000 mA
        Laser bias current high warning threshold : 13.000 mA
        Laser bias current low warning threshold  : 2.000 mA
        Laser output power high alarm threshold   : 1.9952 mW / 3.00 dBm
        Laser output power low alarm threshold    : 0.1584 mW / -8.00 dBm
        Laser output power high warning threshold : 1.5848 mW / 2.00 dBm
        Laser output power low warning threshold  : 0.1778 mW / -7.50 dBm
        Module temperature high alarm threshold   : 80.00 degrees C / 176.00 degrees F
        Module temperature low alarm threshold    : -10.00 degrees C / 14.00 degrees F
        Module temperature high warning threshold : 75.00 degrees C / 167.00 degrees F
        Module temperature low warning threshold  : -5.00 degrees C / 23.00 degrees F
        Module voltage high alarm threshold       : 3.6000 V
        Module voltage low alarm threshold        : 3.0000 V
        Module voltage high warning threshold     : 3.5000 V
        Module voltage low warning threshold      : 3.1000 V
        Laser rx power high alarm threshold       : 1.1220 mW / 0.50 dBm
        Laser rx power low alarm threshold        : 0.0199 mW / -17.01 dBm
        Laser rx power high warning threshold     : 1.0000 mW / 0.00 dBm
        Laser rx power low warning threshold      : 0.0223 mW / -16.52 dBm


root@s508cl:~# ethtool -m lan8
        Identifier                                : 0x03 (SFP)
        Extended identifier                       : 0x04 (GBIC/SFP defined by 2-wire interface ID)
        Connector                                 : 0x07 (LC)
        Transceiver codes                         : 0x10 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
        Transceiver type                          : 10G Ethernet: 10G Base-SR
        Encoding                                  : 0x06 (64B/66B)
        BR, Nominal                               : 10300MBd
        Rate identifier                           : 0x00 (unspecified)
        Length (SMF,km)                           : 0km
        Length (SMF)                              : 0m
        Length (50um)                             : 80m
        Length (62.5um)                           : 20m
        Length (Copper)                           : 0m
        Length (OM3)                              : 300m
        Laser wavelength                          : 0nm
        Vendor name                               : OEM
        Vendor OUI                                : 00:00:00
        Vendor PN                                 : SFP-10G-T8
        Vendor rev                                : A
        Option values                             : 0x00 0x1a
        Option                                    : RX_LOS implemented
        Option                                    : TX_FAULT implemented
        Option                                    : TX_DISABLE implemented
        BR margin, max                            : 0%
        BR margin, min                            : 0%
        Vendor SN                                 : F250114T0010
        Date code                                 : 250115
        Optical diagnostics support               : Yes
        Laser bias current                        : 6.000 mA
        Laser output power                        : 0.5010 mW / -3.00 dBm
        Receiver signal average optical power     : 0.5010 mW / -3.00 dBm
        Module temperature                        : 67.64 degrees C / 153.75 degrees F
        Module voltage                            : 3.2538 V
        Alarm/warning flags implemented           : Yes
        Laser bias current high alarm             : Off
        Laser bias current low alarm              : Off
        Laser bias current high warning           : Off
        Laser bias current low warning            : Off
        Laser output power high alarm             : Off
        Laser output power low alarm              : Off
        Laser output power high warning           : Off
        Laser output power low warning            : Off
        Module temperature high alarm             : Off
        Module temperature low alarm              : Off
        Module temperature high warning           : Off
        Module temperature low warning            : Off
        Module voltage high alarm                 : Off
        Module voltage low alarm                  : Off
        Module voltage high warning               : Off
        Module voltage low warning                : Off
        Laser rx power high alarm                 : Off
        Laser rx power low alarm                  : Off
        Laser rx power high warning               : Off
        Laser rx power low warning                : Off
        Laser bias current high alarm threshold   : 15.000 mA
        Laser bias current low alarm threshold    : 1.000 mA
        Laser bias current high warning threshold : 13.000 mA
        Laser bias current low warning threshold  : 2.000 mA
        Laser output power high alarm threshold   : 1.1220 mW / 0.50 dBm
        Laser output power low alarm threshold    : 0.1862 mW / -7.30 dBm
        Laser output power high warning threshold : 1.0000 mW / 0.00 dBm
        Laser output power low warning threshold  : 0.2344 mW / -6.30 dBm
        Module temperature high alarm threshold   : 80.00 degrees C / 176.00 degrees F
        Module temperature low alarm threshold    : -10.00 degrees C / 14.00 degrees F
        Module temperature high warning threshold : 75.00 degrees C / 167.00 degrees F
        Module temperature low warning threshold  : -5.00 degrees C / 23.00 degrees F
        Module voltage high alarm threshold       : 3.5999 V
        Module voltage low alarm threshold        : 2.9000 V
        Module voltage high warning threshold     : 3.5000 V
        Module voltage low warning threshold      : 3.0000 V
        Laser rx power high alarm threshold       : 1.1220 mW / 0.50 dBm
        Laser rx power low alarm threshold        : 0.0645 mW / -11.90 dBm
        Laser rx power high warning threshold     : 1.0000 mW / 0.00 dBm
        Laser rx power low warning threshold      : 0.0812 mW / -10.90 dBm


Phy access works too:

root@s508cl:~# mdio smbus:sfp-p5 phy 22
BMCR(0x00): 0x1140
  flags: -reset -loopback +aneg-enable -power-down -isolate -aneg-restart
         -collision-test
  speed: 1000-full

BMSR(0x01): 0x7949
  capabilities: -100-t4 +100-tx-f +100-tx-h +10-t-f +10-t-h -100-t2-f -100-t2-h
  flags:        +ext-status -aneg-complete -remote-fault +aneg-capable -link
                -jabber +ext-register

ID(0x02/0x03): 0x01410cc2

ESTATUS(0x0F): 0xf000
  capabilities: +1000-x-f +1000-x-h +1000-t-f +1000-t-h



Even this, which is using RollBall over SMBus:

root@s508cl:~# mdio smbus:sfp-p3 mmd 17:1
CTRL1(0x00): 0x0002
  flags: -reset -low-power +remote-loopback -local-loopback
  speed: 10

STAT1(0x01): 0x0002
  capabilities: -pias -peas +low-power
  flags:        -fault -link

DEVID(0x02/0x03): 0x31c31c12

SPEED(0x04): 0x6031
  capabilities: -400g +5g +2.5g -200g -25g -10g-xr -100g -40g -10g/1g -10 +100
                +1000 -10-ts -2-tl +10g

DEVS(0x06/0x05): 0xe000009a
  devices: +vendor2 +vendor1 +c22-ext -power-unit -ofdm -pma4 -pma3 -pma2 -pma1
           +aneg -tc -dte-xs +phy-xs +pcs -wis +pma/pmd -c22

CTRL2(0x07): 0x0009
  flags: -pias -peas
  type:  10g-t

STAT2(0x08): 0xb301
  capabilities: +tx-fault +rx-fault +ext-register +tx-disable +local-loopback
                -10g-sr -10g-lr -10g-er -10g-lx4 -10g-sw -10g-lw -10g-ew
  flags:        +present -tx-fault -rx-fault

EXTABLE(0x0B): 0x40fc
  capabilities: -10g-cx4 -10g-lrm +10g-t +10g-kx4 +10g-kr +1000-t +1000-kx
                +100-tx -10-t -p2mp -40g/100g -1000/100-t1 -25g -200g/400g
                +2.5g/5g -1000-h

PKGID(0x0E/0x0F): 0x31c31c12


Bjørn

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Sun, Feb 23, 2025 at 07:37:05PM +0100, Bjørn Mork wrote:
> As for Russel's comments regarding atomic reads, I'm hoping for the
> pragmatic approach and allow all possible features over SMBus. It's not
> like we have the option of using i2c on a host which only supports
> SMBus.  My experience is that both hwmon and phy access works pretty
> well with SMBus byte accesses.

The pragmatic approach is to avoid using things that are unsafe (and
thus unreliable.) As I said, disabling hwmon makes sense because you
don't want to read the 16-bit values non-atomically and end up with
scrambled readings that then trigger alarms because they've exceeded
the thresholds.

Merely proving that "oh look it works because I can read stuff"
doesn't address my point.

SMBus being used is as bad as all those crappy SFP modules out there
that don't conform to the SFP MSA but claim they do. It's just yet
another hardware designer cocking the design up in a way that makes
SFPs unreliable. Unfortunately, there's not much that we can do to
influence that, but not publishing stuff that's ultimately unreliable
helps to make the issue known.

So, not only do I think that hwmon should be disabled if using SMBus,
but I also think that the kernel should print a warning that SMBus is
being used and therefore e.g. copper modules will be unreliable. We
don't know how the various firmwares in various microprocessors that
convert I2C to MDIO will behave when faced with SMBus transfers.

All in all, I'm not happy with this, and I do wish hardware designers
would get a clue.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Andrew Lunn]

> So, not only do I think that hwmon should be disabled if using SMBus,
> but I also think that the kernel should print a warning that SMBus is
> being used and therefore e.g. copper modules will be unreliable. We
> don't know how the various firmwares in various microprocessors that
> convert I2C to MDIO will behave when faced with SMBus transfers.

I agree, hwmon should be disabled, and that the kernel should printing
a warning that the hardware is broken and that networking is not
guaranteed to be reliable.

	Andrew

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Bjørn Mork]

Andrew Lunn <andrew@lunn.ch> writes:

>> So, not only do I think that hwmon should be disabled if using SMBus,
>> but I also think that the kernel should print a warning that SMBus is
>> being used and therefore e.g. copper modules will be unreliable. We
>> don't know how the various firmwares in various microprocessors that
>> convert I2C to MDIO will behave when faced with SMBus transfers.
>
> I agree, hwmon should be disabled, and that the kernel should printing
> a warning that the hardware is broken and that networking is not
> guaranteed to be reliable.

What do you think will be the effect of such a warning?  Who is the
target audience?

You can obviously add it, and I don't really care.  But I believe the
result will be an endless stream of end users worrying about this scary
warning and wanting to know what they can do about it.  What will be
your answer?

No SoC/phy designer will ever see the warning.  They finished designing
these chips decades ago.  The switch designers might see it. But
probably not. They're not worried about running mainline Linux at all.
They have they're vendor SDK.  Linux based of course, but it's never
going to have that warning no matter what you do.  Firmware developers?
Same as switch designers really.

The hardware exists. It's not perfect. We agree so far.  But I do not
understand your way of dealing with that.

If your intention is detecting this hardware problem in bug reports etc,
then it makes more sense to me.  But I believe a more subtle method will
be more effeicient than a standalone and scary warning. Like embedding
"smbus" or similar in existing debug/warning/error messages, e.g by
making it part of the mdio bus name.


Bjørn

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Mon, Feb 24, 2025 at 08:13:22AM +0100, Bjørn Mork wrote:
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> So, not only do I think that hwmon should be disabled if using SMBus,
> >> but I also think that the kernel should print a warning that SMBus is
> >> being used and therefore e.g. copper modules will be unreliable. We
> >> don't know how the various firmwares in various microprocessors that
> >> convert I2C to MDIO will behave when faced with SMBus transfers.
> >
> > I agree, hwmon should be disabled, and that the kernel should printing
> > a warning that the hardware is broken and that networking is not
> > guaranteed to be reliable.
> 
> What do you think will be the effect of such a warning?  Who is the
> target audience?
> 
> You can obviously add it, and I don't really care.  But I believe the
> result will be an endless stream of end users worrying about this scary
> warning and wanting to know what they can do about it.  What will be
> your answer?

... which is good, because it raises the visibility of crap hardware
and will make people think twice about whether to purchase it, thus
penalising (a little) the sales of badly designed hardware.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

On Mon, 24 Feb 2025 08:47:18 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Feb 24, 2025 at 08:13:22AM +0100, Bjørn Mork wrote:
> > Andrew Lunn <andrew@lunn.ch> writes:
> >   
> > >> So, not only do I think that hwmon should be disabled if using SMBus,
> > >> but I also think that the kernel should print a warning that SMBus is
> > >> being used and therefore e.g. copper modules will be unreliable. We
> > >> don't know how the various firmwares in various microprocessors that
> > >> convert I2C to MDIO will behave when faced with SMBus transfers.  
> > >
> > > I agree, hwmon should be disabled, and that the kernel should printing
> > > a warning that the hardware is broken and that networking is not
> > > guaranteed to be reliable.  
> > 
> > What do you think will be the effect of such a warning?  Who is the
> > target audience?
> > 
> > You can obviously add it, and I don't really care.  But I believe the
> > result will be an endless stream of end users worrying about this scary
> > warning and wanting to know what they can do about it.  What will be
> > your answer?  
> 
> ... which is good, because it raises the visibility of crap hardware
> and will make people think twice about whether to purchase it, thus
> penalising (a little) the sales of badly designed hardware.

It's also going to allow users to understand a bit more why all
features aren't available. Plugging modules in a true i2c connected
cage will report all the features, while plugging on an smbus cage will
cause the use of a degraded mode. Not only for hwmon, but as Russell
specifies, I doubt all SFP PHY will behave exactly the same...

I'll respin without hwmon and with the warning then.

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Andrew Lunn]

> What do you think will be the effect of such a warning?  Who is the
> target audience?

It will act as a disclaimer. The kernel is doing its best with broken
hardware, but don't blame the kernel when it does not work
correctly....

> You can obviously add it, and I don't really care.  But I believe the
> result will be an endless stream of end users worrying about this scary
> warning and wanting to know what they can do about it.  What will be
> your answer?

I agree that the wording needs to be though about. Maybe something
like:

This hardware is broken by design, and there is nothing the kernel, or
the community can do about it. The kernel will try its best, but some
standard SFP features are disabled, and the features which are
implemented may not work correctly because of the design errors. Use
with caution, and don't blame the kernel when it all goes horribly
wrong.

	Andrew

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Mon, Feb 24, 2025 at 02:31:42PM +0100, Andrew Lunn wrote:
> > What do you think will be the effect of such a warning?  Who is the
> > target audience?
> 
> It will act as a disclaimer. The kernel is doing its best with broken
> hardware, but don't blame the kernel when it does not work
> correctly....

Indeed.

> > You can obviously add it, and I don't really care.  But I believe the
> > result will be an endless stream of end users worrying about this scary
> > warning and wanting to know what they can do about it.  What will be
> > your answer?
> 
> I agree that the wording needs to be though about. Maybe something
> like:
> 
> This hardware is broken by design, and there is nothing the kernel, or
> the community can do about it. The kernel will try its best, but some
> standard SFP features are disabled, and the features which are
> implemented may not work correctly because of the design errors. Use
> with caution, and don't blame the kernel when it all goes horribly
> wrong.

I was hoping for something shorter, but I think it needs to be expansive
so that users can fully understand. Another idea based on your
suggestion above:

"Please note:
This hardware is broken by design. There is nothing that the kernel or
community can do to fix it. The kernel will try best efforts, but some
features are disabled, other features may be unreliable or sporadically
fail. Use with caution. Please verify any problems on hardware that
supports multi-byte I2C transactions."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Andrew Lunn]

> "Please note:
> This hardware is broken by design. There is nothing that the kernel or
> community can do to fix it. The kernel will try best efforts, but some
> features are disabled, other features may be unreliable or sporadically
> fail. Use with caution. Please verify any problems on hardware that
> supports multi-byte I2C transactions."

I'm good with that.

	Andrew

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Marek Behún]

On Mon, Feb 24, 2025 at 01:48:19PM +0000, Russell King (Oracle) wrote:

...

> "Please note:
> This hardware is broken by design. There is nothing that the kernel or

Doesn't "broken by design" mean "broken intentionally" ?

Marek

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Russell King (Oracle)]

On Mon, Feb 24, 2025 at 03:32:43PM +0100, Marek Behún wrote:
> On Mon, Feb 24, 2025 at 01:48:19PM +0000, Russell King (Oracle) wrote:
> 
> ...
> 
> > "Please note:
> > This hardware is broken by design. There is nothing that the kernel or
> 
> Doesn't "broken by design" mean "broken intentionally" ?

The point of this part of the discussion is to find a form of words
that folk are happy with, so please feel free to suggest an
alternative.

To put it bluntly, it does come down to "broken by the hardware
engineer not having full knowledge of the requirements of SFP modules
as documented in the SNIA documentation and selecting hardware that
violates some of the requirements."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

On Mon, 24 Feb 2025 13:48:19 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Feb 24, 2025 at 02:31:42PM +0100, Andrew Lunn wrote:
> > > What do you think will be the effect of such a warning?  Who is the
> > > target audience?  
> > 
> > It will act as a disclaimer. The kernel is doing its best with broken
> > hardware, but don't blame the kernel when it does not work
> > correctly....  
> 
> Indeed.
> 
> > > You can obviously add it, and I don't really care.  But I believe the
> > > result will be an endless stream of end users worrying about this scary
> > > warning and wanting to know what they can do about it.  What will be
> > > your answer?  
> > 
> > I agree that the wording needs to be though about. Maybe something
> > like:
> > 
> > This hardware is broken by design, and there is nothing the kernel, or
> > the community can do about it. The kernel will try its best, but some
> > standard SFP features are disabled, and the features which are
> > implemented may not work correctly because of the design errors. Use
> > with caution, and don't blame the kernel when it all goes horribly
> > wrong.  
> 
> I was hoping for something shorter, but I think it needs to be expansive
> so that users can fully understand. Another idea based on your
> suggestion above:
> 
> "Please note:
> This hardware is broken by design. There is nothing that the kernel or
> community can do to fix it. The kernel will try best efforts, but some
> features are disabled, other features may be unreliable or sporadically
> fail. Use with caution. Please verify any problems on hardware that
> supports multi-byte I2C transactions."
> 

I think what's missing in this message is some indication about what is
actually wrong with the hardware, so :

"Please note:
This SFP cage is accessed via an SMBus only capable of single byte
transactions. Some features are disabled, other may be unreliable or
sporadically fail. Use with caution. There is nothing that the kernel
or community can do to fix it, the kernel will try best efforts. Please
verify any problems on hardware that supports multi-byte I2C transactions."

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

On Mon, 24 Feb 2025 17:24:07 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> On Mon, 24 Feb 2025 13:48:19 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Feb 24, 2025 at 02:31:42PM +0100, Andrew Lunn wrote:  
> > > > What do you think will be the effect of such a warning?  Who is the
> > > > target audience?    
> > > 
> > > It will act as a disclaimer. The kernel is doing its best with broken
> > > hardware, but don't blame the kernel when it does not work
> > > correctly....    
> > 
> > Indeed.
> >   
> > > > You can obviously add it, and I don't really care.  But I believe the
> > > > result will be an endless stream of end users worrying about this scary
> > > > warning and wanting to know what they can do about it.  What will be
> > > > your answer?    
> > > 
> > > I agree that the wording needs to be though about. Maybe something
> > > like:
> > > 
> > > This hardware is broken by design, and there is nothing the kernel, or
> > > the community can do about it. The kernel will try its best, but some
> > > standard SFP features are disabled, and the features which are
> > > implemented may not work correctly because of the design errors. Use
> > > with caution, and don't blame the kernel when it all goes horribly
> > > wrong.    
> > 
> > I was hoping for something shorter, but I think it needs to be expansive
> > so that users can fully understand. Another idea based on your
> > suggestion above:
> > 
> > "Please note:
> > This hardware is broken by design. There is nothing that the kernel or
> > community can do to fix it. The kernel will try best efforts, but some
> > features are disabled, other features may be unreliable or sporadically
> > fail. Use with caution. Please verify any problems on hardware that
> > supports multi-byte I2C transactions."
> >   
> 
> I think what's missing in this message is some indication about what is
> actually wrong with the hardware, so :

I realise that I have formulated the sentence above a bit strongly, of
course this is a suggestion :)

> "Please note:
> This SFP cage is accessed via an SMBus only capable of single byte
> transactions. Some features are disabled, other may be unreliable or
> sporadically fail. Use with caution. There is nothing that the kernel
> or community can do to fix it, the kernel will try best efforts. Please
> verify any problems on hardware that supports multi-byte I2C transactions."

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Maxime Chevallier]

Hi Bjørn,

On Sun, 23 Feb 2025 19:37:05 +0100
Bjørn Mork <bjorn@mork.no> wrote:

> Maxime Chevallier <maxime.chevallier@bootlin.com> writes:
> 
> > Hi everyone,
> >
> > Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> > access SFP modules downstream. These controllers are actually SMBus controllers
> > that can only perform single-byte accesses for read and write.
> >
> > This series adds support for accessing SFP modules through single-byte SMBus,
> > which could be relevant for other setups.
> >
> > The first patch deals with the SFP module access by itself, for addresses 0x50
> > and 0x51.
> >
> > The second patch allows accessing embedded PHYs within the module with single-byte
> > SMBus, adding this in the mdio-i2c driver.
> >
> > As raw i2c transfers are always more efficient, we make sure that the smbus accesses
> > are only used if we really have no other choices.
> >
> > This has been tested with the following modules (as reported upon module insertion)
> >
> > Fiber modules :
> >
> > 	UBNT             UF-MM-1G         rev      sn FT20051201212    dc 200512
> > 	PROLABS          SFP-1GSXLC-T-C   rev A1   sn PR2109CA1080     dc 220607
> > 	CISCOSOLIDOPTICS CWDM-SFP-1490    rev 1.0  sn SOSC49U0891      dc 181008
> > 	CISCOSOLIDOPTICS CWDM-SFP-1470    rev 1.0  sn SOSC47U1175      dc 190620
> > 	OEM              SFP-10G-SR       rev 02   sn CSSSRIC3174      dc 181201
> > 	FINISAR CORP.    FTLF1217P2BTL-HA rev A    sn PA3A0L6          dc 230716
> > 	OEM              ES8512-3LCD05    rev 10   sn ESC22SX296055    dc 220722
> > 	SOURCEPHOTONICS  SPP10ESRCDFF     rev 10   sn E8G2017450       dc 140715
> > 	CXR              SFP-STM1-MM-850  rev 0000 sn K719017031       dc 200720
> >
> >  Copper modules
> >
> > 	OEM              SFT-7000-RJ45-AL rev 11.0 sn EB1902240862     dc 190313
> > 	FINISAR CORP.    FCLF8521P2BTL    rev A    sn P1KBAPD          dc 190508
> > 	CHAMPION ONE     1000SFPT         rev -    sn     GBC59750     dc 19110401
> >
> > DAC :
> >
> > 	OEM              SFP-H10GB-CU1M   rev R    sn CSC200803140115  dc 200827
> >
> > In all cases, read/write operations happened without errors, and the internal
> > PHY (if any) was always properly detected and accessible
> >
> > I haven't tested with any RollBall SFPs though, as I don't have any, and I don't
> > have Copper modules with anything else than a Marvell 88e1111 inside. The support
> > for the VSC8552 SMBus may follow at some point.
> >
> > Thanks,
> >
> > Maxime
> >
> > Maxime Chevallier (2):
> >   net: phy: sfp: Add support for SMBus module access
> >   net: mdio: mdio-i2c: Add support for single-byte SMBus operations
> >
> >  drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
> >  drivers/net/phy/sfp.c       | 65 +++++++++++++++++++++++++++---
> >  2 files changed, 138 insertions(+), 6 deletions(-)  
> 
> Nice!  Don't know if you're aware, but OpenWrt have had patches for
> SMBus access to SFPs for some time:
> 
> https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/714-net-phy-sfp-add-support-for-SMBus.patch
> https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/712-net-phy-add-an-MDIO-SMBus-library.patch
> 
> The reason they carry these is that they support Realtek rtl930x based
> switches.  The rtl930x SoCs include an 8 channel SMBus host which is
> typically connected to any SFP+ slots on the switch.
> 
> There has been work going on for a while to bring the support for these
> SoCs to mainline, and the SMBus host driver is already here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-rtl9300.c?id=c366be720235301fdadf67e6f1ea6ff32669c074
> 
> I assume DSA and ethernet eventually will follow, making SMBus SFP
> support necessary for this platform too.
> 
> So thanks for doing this!

Good to know this is useful to you ! So there's at least 2 different
classes of products out there with SMBus that advertise that it's
"designed for SFP" ._.

> FWIW, I don't think the OpenWrt mdio patch works at all.  I've recently
> been playing with an 8 SFP+ port switch based on rtl9303, and have tried
> to fixup both the clause 22 support and add RollBall and clause 45.
> This is still a somewhat untested hack, and I was not planning on
> presenting it here as such, but since this discussion is open:
> https://github.com/openwrt/openwrt/pull/17950/commits/c40387104af62a065797bc3e23dfb9f36e03851b
> 
> Sorry for the format.  This is a patch for the patch already present in
> OpenWrt. Let me know if you want me to post the complete patched
> mdio-smbus.c for easier reading.
> 
> The main point I wanted to make is that we also need RollBall and clause
> 45 over SMBus.  Maybe not today, but at some point.  Ideally, the code
> should be shared with the i2c implementation, but I found that very hard
> to do as it is.

I don't have anything to test that, and yeah that can be considered as
a second step, however I don't even know if this can work at all with
single byte accesses :(

Thanks,

Maxime

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Marek Behún]

On Mon, Feb 24, 2025 at 10:38:14AM +0100, Maxime Chevallier wrote:
> Hi Bjørn,
> 
> On Sun, 23 Feb 2025 19:37:05 +0100
> Bjørn Mork <bjorn@mork.no> wrote:
> 
> > Maxime Chevallier <maxime.chevallier@bootlin.com> writes:
> > 
> > > Hi everyone,
> > >
> > > Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> > > access SFP modules downstream. These controllers are actually SMBus controllers
> > > that can only perform single-byte accesses for read and write.
> > >
> > > This series adds support for accessing SFP modules through single-byte SMBus,
> > > which could be relevant for other setups.
> > >
> > > The first patch deals with the SFP module access by itself, for addresses 0x50
> > > and 0x51.
> > >
> > > The second patch allows accessing embedded PHYs within the module with single-byte
> > > SMBus, adding this in the mdio-i2c driver.
> > >
> > > As raw i2c transfers are always more efficient, we make sure that the smbus accesses
> > > are only used if we really have no other choices.
> > >
> > > This has been tested with the following modules (as reported upon module insertion)
> > >
> > > Fiber modules :
> > >
> > > 	UBNT             UF-MM-1G         rev      sn FT20051201212    dc 200512
> > > 	PROLABS          SFP-1GSXLC-T-C   rev A1   sn PR2109CA1080     dc 220607
> > > 	CISCOSOLIDOPTICS CWDM-SFP-1490    rev 1.0  sn SOSC49U0891      dc 181008
> > > 	CISCOSOLIDOPTICS CWDM-SFP-1470    rev 1.0  sn SOSC47U1175      dc 190620
> > > 	OEM              SFP-10G-SR       rev 02   sn CSSSRIC3174      dc 181201
> > > 	FINISAR CORP.    FTLF1217P2BTL-HA rev A    sn PA3A0L6          dc 230716
> > > 	OEM              ES8512-3LCD05    rev 10   sn ESC22SX296055    dc 220722
> > > 	SOURCEPHOTONICS  SPP10ESRCDFF     rev 10   sn E8G2017450       dc 140715
> > > 	CXR              SFP-STM1-MM-850  rev 0000 sn K719017031       dc 200720
> > >
> > >  Copper modules
> > >
> > > 	OEM              SFT-7000-RJ45-AL rev 11.0 sn EB1902240862     dc 190313
> > > 	FINISAR CORP.    FCLF8521P2BTL    rev A    sn P1KBAPD          dc 190508
> > > 	CHAMPION ONE     1000SFPT         rev -    sn     GBC59750     dc 19110401
> > >
> > > DAC :
> > >
> > > 	OEM              SFP-H10GB-CU1M   rev R    sn CSC200803140115  dc 200827
> > >
> > > In all cases, read/write operations happened without errors, and the internal
> > > PHY (if any) was always properly detected and accessible
> > >
> > > I haven't tested with any RollBall SFPs though, as I don't have any, and I don't
> > > have Copper modules with anything else than a Marvell 88e1111 inside. The support
> > > for the VSC8552 SMBus may follow at some point.
> > >
> > > Thanks,
> > >
> > > Maxime
> > >
> > > Maxime Chevallier (2):
> > >   net: phy: sfp: Add support for SMBus module access
> > >   net: mdio: mdio-i2c: Add support for single-byte SMBus operations
> > >
> > >  drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
> > >  drivers/net/phy/sfp.c       | 65 +++++++++++++++++++++++++++---
> > >  2 files changed, 138 insertions(+), 6 deletions(-)  
> > 
> > Nice!  Don't know if you're aware, but OpenWrt have had patches for
> > SMBus access to SFPs for some time:
> > 
> > https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/714-net-phy-sfp-add-support-for-SMBus.patch
> > https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/712-net-phy-add-an-MDIO-SMBus-library.patch
> > 
> > The reason they carry these is that they support Realtek rtl930x based
> > switches.  The rtl930x SoCs include an 8 channel SMBus host which is
> > typically connected to any SFP+ slots on the switch.
> > 
> > There has been work going on for a while to bring the support for these
> > SoCs to mainline, and the SMBus host driver is already here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-rtl9300.c?id=c366be720235301fdadf67e6f1ea6ff32669c074
> > 
> > I assume DSA and ethernet eventually will follow, making SMBus SFP
> > support necessary for this platform too.
> > 
> > So thanks for doing this!
> 
> Good to know this is useful to you ! So there's at least 2 different
> classes of products out there with SMBus that advertise that it's
> "designed for SFP" ._.
> 
> > FWIW, I don't think the OpenWrt mdio patch works at all.  I've recently
> > been playing with an 8 SFP+ port switch based on rtl9303, and have tried
> > to fixup both the clause 22 support and add RollBall and clause 45.
> > This is still a somewhat untested hack, and I was not planning on
> > presenting it here as such, but since this discussion is open:
> > https://github.com/openwrt/openwrt/pull/17950/commits/c40387104af62a065797bc3e23dfb9f36e03851b
> > 
> > Sorry for the format.  This is a patch for the patch already present in
> > OpenWrt. Let me know if you want me to post the complete patched
> > mdio-smbus.c for easier reading.
> > 
> > The main point I wanted to make is that we also need RollBall and clause
> > 45 over SMBus.  Maybe not today, but at some point.  Ideally, the code
> > should be shared with the i2c implementation, but I found that very hard
> > to do as it is.
> 
> I don't have anything to test that, and yeah that can be considered as
> a second step, however I don't even know if this can work at all with
> single byte accesses :(

I will send you some RollBall modules, Maxime.

Marek

Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Sean Anderson]

On 2/23/25 12:28, Maxime Chevallier wrote:
> Hi everyone,
> 
> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> access SFP modules downstream. These controllers are actually SMBus controllers
> that can only perform single-byte accesses for read and write.
> 
> This series adds support for accessing SFP modules through single-byte SMBus,
> which could be relevant for other setups.
> 
> The first patch deals with the SFP module access by itself, for addresses 0x50
> and 0x51.
> 
> The second patch allows accessing embedded PHYs within the module with single-byte
> SMBus, adding this in the mdio-i2c driver.
> 
> As raw i2c transfers are always more efficient, we make sure that the smbus accesses
> are only used if we really have no other choices.
> 
> This has been tested with the following modules (as reported upon module insertion)
> 
> Fiber modules :
> 
> 	UBNT             UF-MM-1G         rev      sn FT20051201212    dc 200512
> 	PROLABS          SFP-1GSXLC-T-C   rev A1   sn PR2109CA1080     dc 220607
> 	CISCOSOLIDOPTICS CWDM-SFP-1490    rev 1.0  sn SOSC49U0891      dc 181008
> 	CISCOSOLIDOPTICS CWDM-SFP-1470    rev 1.0  sn SOSC47U1175      dc 190620
> 	OEM              SFP-10G-SR       rev 02   sn CSSSRIC3174      dc 181201
> 	FINISAR CORP.    FTLF1217P2BTL-HA rev A    sn PA3A0L6          dc 230716
> 	OEM              ES8512-3LCD05    rev 10   sn ESC22SX296055    dc 220722
> 	SOURCEPHOTONICS  SPP10ESRCDFF     rev 10   sn E8G2017450       dc 140715
> 	CXR              SFP-STM1-MM-850  rev 0000 sn K719017031       dc 200720
> 
>  Copper modules
> 
> 	OEM              SFT-7000-RJ45-AL rev 11.0 sn EB1902240862     dc 190313
> 	FINISAR CORP.    FCLF8521P2BTL    rev A    sn P1KBAPD          dc 190508
> 	CHAMPION ONE     1000SFPT         rev -    sn     GBC59750     dc 19110401
> 
> DAC :
> 
> 	OEM              SFP-H10GB-CU1M   rev R    sn CSC200803140115  dc 200827
> 
> In all cases, read/write operations happened without errors, and the internal
> PHY (if any) was always properly detected and accessible
> 
> I haven't tested with any RollBall SFPs though, as I don't have any, and I don't
> have Copper modules with anything else than a Marvell 88e1111 inside. The support
> for the VSC8552 SMBus may follow at some point.
> 
> Thanks,
> 
> Maxime
> 
> Maxime Chevallier (2):
>   net: phy: sfp: Add support for SMBus module access
>   net: mdio: mdio-i2c: Add support for single-byte SMBus operations
> 
>  drivers/net/mdio/mdio-i2c.c | 79 ++++++++++++++++++++++++++++++++++++-
>  drivers/net/phy/sfp.c       | 65 +++++++++++++++++++++++++++---
>  2 files changed, 138 insertions(+), 6 deletions(-)
> 

For this series:

Tested-by: Sean Anderson <sean.anderson@linux.dev>

With a

FS               SFP-GB-GE-T      rev F    sn F2030222359      dc 200729

See [1] for the original "bug report." Note that as discussed later in
the thread, this is a Fiber Store (fs.com) module and not a Finisar one.

--Sean

[1] https://lore.kernel.org/netdev/55f6cec4-2497-45a4-cb1a-3edafa7d80d3@seco.com/