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

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

[Maxime Chevallier]

Hello everyone,

This is V3 for the single-byte SMBus support for SFP cages as well as
embedded PHYs accessed over mdio-i2c.

Discussions on the previous iteration raised some questions on the
ordering of the accesses (from Andrew), after further testing it still
seems correct to me, I can properly read EEPROMS and by hacking around,
read 16 bits values (although this is disabled anyways).

This series compared to V2 changes the approach used internally to use
the degraded mode (no hwmon), by setting the i2c_block_size to 1 and
re-using the logic already in place to deal with that.

As we now have 2 scenarios for 1-byte accesses (broken EEPROM that needs
to be accessed 1 byte at a time or single-byte SMBus), I've updated some
of the in-place error logs. Users should still be plenty warned and
made aware of the situation.

I also renamed the internal helpers to reflect that we use single-byte
accesses.

The rest was left untouched (I didn't have time yet to test around with
Rollball for example or 16-bits SMBus).

As I reworked patch 1, I dropped Sean's tested-by :(

V2 : https://lore.kernel.org/netdev/20250225112043.419189-1-maxime.chevallier@bootlin.com/
V1 : https://lore.kernel.org/netdev/20250223172848.1098621-1-maxime.chevallier@bootlin.com/#t

@Maintainers: I already have a few series queued for review, let me know
if you prefer that I resend that one at a later time.

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       | 82 +++++++++++++++++++++++++++++++++----
 2 files changed, 152 insertions(+), 9 deletions(-)

-- 
2.48.1

[PATCH net-next v3 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.

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.

As Single-byte SMBus transaction go against SFF-8472 and breaks the
atomicity for diagnostics data access, hwmon is disabled in the case
of SMBus access.

Moreover, as this may cause other instabilities, print a warning at
probe time to indicate that the setup may be unreliable because of the
hardware design.

As hwmon may be disabled for both broken EEPROM and smbus, the warnings
are udpated accordingly.

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

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index c88217af44a1..cd7624a26cd4 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -234,6 +234,7 @@ struct sfp {
 	enum mdio_i2c_proto mdio_protocol;
 	struct phy_device *mod_phy;
 	const struct sff_data *type;
+	size_t i2c_max_block_size;
 	size_t i2c_block_size;
 	u32 max_power_mW;
 
@@ -691,14 +692,71 @@ 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_byte_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_byte_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;
+		sfp->i2c_max_block_size = SFP_EEPROM_BLOCK_SIZE;
+	} else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		sfp->read = sfp_smbus_byte_read;
+		sfp->write = sfp_smbus_byte_write;
+		sfp->i2c_max_block_size = 1;
+	} else {
+		sfp->i2c = NULL;
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1594,7 +1652,7 @@ static void sfp_hwmon_probe(struct work_struct *work)
 	 */
 	if (sfp->i2c_block_size < 2) {
 		dev_info(sfp->dev,
-			 "skipping hwmon device registration due to broken EEPROM\n");
+			 "skipping hwmon device registration\n");
 		dev_info(sfp->dev,
 			 "diagnostic EEPROM area cannot be read atomically to guarantee data coherency\n");
 		return;
@@ -2201,7 +2259,7 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	sfp->i2c_block_size = SFP_EEPROM_BLOCK_SIZE;
+	sfp->i2c_block_size = sfp->i2c_max_block_size;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -2941,7 +2999,6 @@ static struct sfp *sfp_alloc(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	sfp->dev = dev;
-	sfp->i2c_block_size = SFP_EEPROM_BLOCK_SIZE;
 
 	mutex_init(&sfp->sm_mutex);
 	mutex_init(&sfp->st_mutex);
@@ -3115,6 +3172,15 @@ static int sfp_probe(struct platform_device *pdev)
 	if (!sfp->sfp_bus)
 		return -ENOMEM;
 
+	if (sfp->i2c_max_block_size < 2)
+		dev_warn(sfp->dev,
+			 "Please note:\n"
+			 "This SFP cage is accessed via an SMBus only capable of single byte\n"
+			 "transactions. Some features are disabled, other may be unreliable or\n"
+			 "sporadically fail. Use with caution. There is nothing that the kernel\n"
+			 "or community can do to fix it, the kernel will try best efforts. Please\n"
+			 "verify any problems on hardware that supports multi-byte I2C transactions.\n");
+
 	sfp_debugfs_init(sfp);
 
 	return 0;
-- 
2.48.1

[PATCH net-next v3 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.

Tested-by: Sean Anderson <sean.anderson@linux.dev>
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 v3 0/2] net: phy: sfp: Add single-byte SMBus SFP access

[Andrew Lunn]

On Fri, Mar 14, 2025 at 05:23:16PM +0100, Maxime Chevallier wrote:
> Hello everyone,
> 
> This is V3 for the single-byte SMBus support for SFP cages as well as
> embedded PHYs accessed over mdio-i2c.

Just curious, what hardware is this? And does it support bit-banging
the I2C pins? If it does, you get a choice, slow but correct vs fast
but broken and limited?

	Andrew

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

[Maxime Chevallier]

Hello Andrew,

On Mon, 17 Mar 2025 22:34:09 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Mar 14, 2025 at 05:23:16PM +0100, Maxime Chevallier wrote:
> > Hello everyone,
> > 
> > This is V3 for the single-byte SMBus support for SFP cages as well as
> > embedded PHYs accessed over mdio-i2c.  
> 
> Just curious, what hardware is this? And does it support bit-banging
> the I2C pins? If it does, you get a choice, slow but correct vs fast
> but broken and limited?

The HW is a VSC8552 PHY that includes a so-called "i2c mux", which in
reality is that smbus interface.

             +---------+
 +-----+     |         |     +-----+
 | MAC | --- | VSC8552 | --- | SFP |
 +-----+     |         |     +-----+
    |        |         |        |
    +-mdio---|         |-smbus--+
             +---------+

it has 4 SCL and 1 SDA lines, that you can connect to 4 different SFP
cages.

You perform transfers by using 2 dedicated MDIO registers , one
register contains xfer info such as the address to access over smbus,
the direction of the xfer, and the other one contains data :
 - lower byte is write data
 - upper byte is read-back data

and that's all you have :( so the HW can only really do one single byte
transfer at a time, then you re-configure the 2 registers above, rinse
and repeat.

Looks like the datasheet is publicly available :

https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001809A.pdf

The whole xfer protocol is described in page 35.

On the board itself, the i2c for the SFP cage is connected to that PHY
smbus.

Now it looks like there's some pinmux within the PHY and we can use the
PHY as a gpio controller, so we could consider using a bitbang approach
indeed (provided that SFP is on PHY smbus bus 0 or 1).

I didn't consider that, it's probably worth giving a try, even if as
you say it's probably be very slow, each bit being set amounting to a
mdio xfer towards the PHY.

But if it allows better HW support, and the SFP reacts well on slow
busses, it may be work :)

Do we still want the current series ? Looks like some other people were
interested in that.

On my side that's the second time I deal with a product that uses a PHY
from this family and uses that smbus feature (but if that bitbang thing
works it's probably better)

Thanks,

Maxime

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

[Andrew Lunn]

On Tue, Mar 18, 2025 at 09:25:51AM +0100, Maxime Chevallier wrote:
> Hello Andrew,
> 
> On Mon, 17 Mar 2025 22:34:09 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Fri, Mar 14, 2025 at 05:23:16PM +0100, Maxime Chevallier wrote:
> > > Hello everyone,
> > > 
> > > This is V3 for the single-byte SMBus support for SFP cages as well as
> > > embedded PHYs accessed over mdio-i2c.  
> > 
> > Just curious, what hardware is this? And does it support bit-banging
> > the I2C pins? If it does, you get a choice, slow but correct vs fast
> > but broken and limited?
> 
> The HW is a VSC8552 PHY that includes a so-called "i2c mux", which in
> reality is that smbus interface.
> 
>              +---------+
>  +-----+     |         |     +-----+
>  | MAC | --- | VSC8552 | --- | SFP |
>  +-----+     |         |     +-----+
>     |        |         |        |
>     +-mdio---|         |-smbus--+
>              +---------+
> 
> it has 4 SCL and 1 SDA lines, that you can connect to 4 different SFP
> cages.
> 
> You perform transfers by using 2 dedicated MDIO registers , one
> register contains xfer info such as the address to access over smbus,
> the direction of the xfer, and the other one contains data :
>  - lower byte is write data
>  - upper byte is read-back data
> 
> and that's all you have :( so the HW can only really do one single byte
> transfer at a time, then you re-configure the 2 registers above, rinse
> and repeat.
> 
> Looks like the datasheet is publicly available :
> 
> https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001809A.pdf
> 
> The whole xfer protocol is described in page 35.
> 
> On the board itself, the i2c for the SFP cage is connected to that PHY
> smbus.
> 
> Now it looks like there's some pinmux within the PHY and we can use the
> PHY as a gpio controller, so we could consider using a bitbang approach
> indeed (provided that SFP is on PHY smbus bus 0 or 1).
> 
> I didn't consider that, it's probably worth giving a try, even if as
> you say it's probably be very slow, each bit being set amounting to a
> mdio xfer towards the PHY.

This going to be very slow. My guess is people will live with limited
functionality. But it could be interesting to implement the GPIO
support and see how slow it is.

It might also be worth pointing out to microchip how broken this is,
and see if they can do anything about it in the firmware running in
PHY. 2 byte SMBUS would solve the problems.

> Do we still want the current series ? Looks like some other people were
> interested in that.

Yes, it is useful.

	Andrew

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

[Paolo Abeni]

On 3/14/25 5:23 PM, Maxime Chevallier wrote:
> @@ -691,14 +692,71 @@ 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_byte_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;

Minor nit: please respect the reverse christmas tree order above.

> +	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_byte_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;

same here.

Thanks,

Paolo

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

[Paolo Abeni]

On 3/14/25 5:23 PM, Maxime Chevallier wrote:
> 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);

External brackets not needed.

> +
> +	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);

same here.

> +
> +	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);

and here.

> +
> +	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;

I would not have noted the above if even this one carried additional
brackets...

Cheers,

Paolo

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

[Maxime Chevallier]

Hi Paolo,

On Fri, 21 Mar 2025 18:50:54 +0100
Paolo Abeni <pabeni@redhat.com> wrote:

> On 3/14/25 5:23 PM, Maxime Chevallier wrote:
> > @@ -691,14 +692,71 @@ 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_byte_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;  
> 
> Minor nit: please respect the reverse christmas tree order above.
> 
> > +	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_byte_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;  
> 
> same here.

Missed it indeed, I'll address that.

Maxime

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

[Maxime Chevallier]

On Fri, 21 Mar 2025 18:53:51 +0100
Paolo Abeni <pabeni@redhat.com> wrote:

> On 3/14/25 5:23 PM, Maxime Chevallier wrote:
> > 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);  
> 
> External brackets not needed.
> 
> > +
> > +	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);  
> 
> same here.
> 
> > +
> > +	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);  
> 
> and here.
> 
> > +
> > +	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;  
> 
> I would not have noted the above if even this one carried additional
> brackets...

:( You're correct, sorry not to have spotted that before... I'll fix
this for v4.

Maxime

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

[Maxime Chevallier]

On Fri, 14 Mar 2025 17:23:18 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> 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.

As a side note, it's kind of a strange coincidence but I just had
access to a weird SGMII to 100BaseFX module (so with a PHY), and from
my tests the PHY only responds to single-byte MDIO accesses !

Trying to access the PHY with word transactions on 0x56 actually causes
the i2c bus to lock-up...

For the curious the module is a CISCO-PROLABS   GLC-GE-100FX-C, and the
PHY id indicates it embeds a Broadcom BCM5461, probably strapped in
SGMII to 100FX mode.

The EEPROM reports strange things though, and I can't get that module to
work at all, the SGMII autoneg appears to go wrong and I get link
up/down events all over the place without anything ever going through,
so I don't think I'll upstream the fixups for the module. Still it
may be another use-case for single-byte mdio-smbus.

Maxime