[net-next,1/5] net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules

Some multigig SFPs from RollBall and Hilink do not expose functional
MDIO access to the internal PHY of the SFP via I2C address 0x56
(although there seems to be read-only clause 22 access on this address).

Instead these SFPs PHY can be accessed via I2C via the SFP Enhanced
Digital Diagnostic Interface - I2C address 0x51.

This extends the mdio-i2c driver to support this protocol by adding a
special parameter to mdio_i2c_alloc function via which this RollBall
protocol can be selected.

Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/mdio/mdio-i2c.c   | 180 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/sfp.c         |   2 +-
 include/linux/mdio/mdio-i2c.h |   8 +-
 3 files changed, 183 insertions(+), 7 deletions(-)

On Wed, Oct 28, 2020 at 11:14:23PM +0100, Marek Behún wrote:
> Some multigig SFPs from RollBall and Hilink do not expose functional
> MDIO access to the internal PHY of the SFP via I2C address 0x56
> (although there seems to be read-only clause 22 access on this address).
> 
> Instead these SFPs PHY can be accessed via I2C via the SFP Enhanced
> Digital Diagnostic Interface - I2C address 0x51.
> 
> This extends the mdio-i2c driver to support this protocol by adding a
> special parameter to mdio_i2c_alloc function via which this RollBall
> protocol can be selected.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>

I think this is probably a better way forward, and I suspect we're going
to see more of this stuff. I wonder, however, whether the configuration
should be done here too (selecting page 1). Also, shouldn't we ensure
that we are on page 1 before attempting any access?

It would be good to pass this through checkpatch - I notice some lines
seem to be over the 80 character limit now.

A few comments below...

> +static int i2c_rollball_mii_poll(struct mii_bus *bus, int bus_addr, u8 *buf, size_t len)
> +{
> +	struct i2c_adapter *i2c = bus->priv;
> +	struct i2c_msg msgs[2];
> +	u8 buf0[2], *res;
> +	int i, ret;
> +
> +	buf0[0] = ROLLBALL_CMD_ADDR;
> +
> +	msgs[0].addr = bus_addr;
> +	msgs[0].flags = 0;
> +	msgs[0].len = 1;
> +	msgs[0].buf = &buf0[0];
> +
> +	res = buf ? buf : &buf0[1];
> +
> +	msgs[1].addr = bus_addr;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].len = buf ? len : 1;
> +	msgs[1].buf = res;
> +
> +	/* By experiment it takes up to 70 ms to access a register for these SFPs. Sleep 20ms
> +	 * between iteratios and try 10 times.

"iterations"

> +static int i2c_mii_read_rollball(struct mii_bus *bus, int phy_id, int reg)
> +{
> +	u8 buf[4], res[6];
> +	int bus_addr, ret;
> +	u16 val;
> +
> +	if (!(reg & MII_ADDR_C45))
> +		return -EOPNOTSUPP;
> +
> +	bus_addr = i2c_mii_phy_addr(phy_id);
> +	if (bus_addr != ROLLBALL_PHY_I2C_ADDR)
> +		return 0xffff;
> +
> +	buf[0] = ROLLBALL_DATA_ADDR;
> +	buf[1] = (reg >> 16) & 0x1f;
> +	buf[2] = (reg >> 8) & 0xff;
> +	buf[3] = reg & 0xff;
> +
> +	ret = i2c_rollball_mii_cmd(bus, bus_addr, ROLLBALL_CMD_READ, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_rollball_mii_poll(bus, bus_addr, res, sizeof(res));
> +	if (ret == -ETIMEDOUT)
> +		return 0xffff;
> +	else if (ret < 0)
> +		return ret;
> +
> +	val = res[4];
> +	val <<= 8;
> +	val |= res[5];

Was there something wrong with:

	val = res[4] << 8 | res[5];

here?

> +struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
> +			       enum mdio_i2c_type type)

Maybe call this "protocol" rather than "type" ?

> +enum mdio_i2c_type {
> +	MDIO_I2C_DEFAULT,
> +	MDIO_I2C_ROLLBALL,
> +};
> +
> +struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
> +			       enum mdio_i2c_type type);

Same here (protocol vs type).

Otherwise, I don't see much else wrong.

> It would be good to pass this through checkpatch - I notice some lines
> seem to be over the 80 character limit now.

Hi Russell

The limit got raised to something higher. I personally prefer 80, and
if a file is using 80, new code would be inconsistent with old code if
it did not use 80. So your request is reasonable anyway.

   Andrew

On Thu, Oct 29, 2020 at 01:54:39PM +0100, Andrew Lunn wrote:
> > It would be good to pass this through checkpatch - I notice some lines
> > seem to be over the 80 character limit now.
> 
> Hi Russell
> 
> The limit got raised to something higher. I personally prefer 80, and
> if a file is using 80, new code would be inconsistent with old code if
> it did not use 80. So your request is reasonable anyway.

Hi Andrew,

I do most of my kernel hacking on the Linux console, so 80x25 is the
limit of what I see - and depending on the editor I'm using, lines
longer than 80 characters are chopped to 80 characters unless I scroll
to the right (which then makes moving up and down problematical.) So,
the files I'm responsible for are likely to stay requiring an 80
character width.

On Thu, 29 Oct 2020 12:41:41 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> I think this is probably a better way forward, and I suspect we're going
> to see more of this stuff. I wonder, however, whether the configuration
> should be done here too (selecting page 1). Also, shouldn't we ensure
> that we are on page 1 before attempting any access?

Very well.

> It would be good to pass this through checkpatch - I notice some lines
> seem to be over the 80 character limit now.

Checkpatch does not complain at over 80 characters anymore, but over
100. But I will rewrite it.

> "iterations"

/o\ thx

> > +	val = res[4];
> > +	val <<= 8;
> > +	val |= res[5];  
> 
> Was there something wrong with:
> 
> 	val = res[4] << 8 | res[5];
> 
> here?

For some reason I preferred to use the 3-liner, but I shall rewrite it.

> 
> > +struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
> > +			       enum mdio_i2c_type type)  
> 
> Maybe call this "protocol" rather than "type" ?

Very well, thx

On Thu, 29 Oct 2020 13:41:07 +0000 Russell King - ARM Linux admin wrote:
> On Thu, Oct 29, 2020 at 01:54:39PM +0100, Andrew Lunn wrote:
> > > It would be good to pass this through checkpatch - I notice some lines
> > > seem to be over the 80 character limit now.  
> > 
> > Hi Russell
> > 
> > The limit got raised to something higher. I personally prefer 80, and
> > if a file is using 80, new code would be inconsistent with old code if
> > it did not use 80. So your request is reasonable anyway.  
> 
> Hi Andrew,
> 
> I do most of my kernel hacking on the Linux console, so 80x25 is the
> limit of what I see - and depending on the editor I'm using, lines
> longer than 80 characters are chopped to 80 characters unless I scroll
> to the right (which then makes moving up and down problematical.) So,
> the files I'm responsible for are likely to stay requiring an 80
> character width.

+1

Maybe we should patch checkpatch to still enforce 80 for networking.

On Thu, 29 Oct 2020 15:53:48 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> +1
> 
> Maybe we should patch checkpatch to still enforce 80 for networking.

If you do, do that for leds as well, Pavel will be glad.