mbox series

[russell-kings-net-queue,v2,0/3] Support for RollBall 10G copper SFP modules

Message ID 20201020150615.11969-1-kabel@kernel.org
Headers show
Series Support for RollBall 10G copper SFP modules | expand

Message

Marek Behún Oct. 20, 2020, 3:06 p.m. UTC 
Hi Russell,

this series should apply on linux-arm git repository, on branch
net-queue.

Some internet providers are already starting to offer 2.5G copper
connectivity to their users. On Turris Omnia the SFP port is capable
of 2.5G speed, so we tested some copper SFP modules.

This adds support to the SFP subsystem for 10G RollBall copper modules
which contain a Marvell 88X3310 PHY. By default these modules are
configured in 10GKR only mode on the host interface, and also contain
some bad information in EEPROM (the extended_cc byte).

The PHY in these modules is also accessed via a different I2C protocol
than the standard one.

Patch 1 adds support for this different I2C MDIO bus.
Patch 2 adds support for these modules into the SFP driver.
Patch 3 changes phylink code so that a PHY can be attached even though
802.3z mode is requested.

Marek

Marek Behún (3):
  net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules
  net: phy: sfp: add support for multigig RollBall modules
  net: phylink: don't fail attaching phy on 1000base-x/2500base-x mode

 drivers/net/phy/mdio-i2c.c | 196 +++++++++++++++++++++++++++++++++++--
 drivers/net/phy/phylink.c  |   4 +-
 drivers/net/phy/sfp.c      |  69 +++++++++++--
 3 files changed, 250 insertions(+), 19 deletions(-)


base-commit: a32e90737c1c92653767d3c95c63c16b9b72c6c2

Comments

Andrew Lunn Oct. 20, 2020, 3:51 p.m. UTC | #1 
> @@ -2006,6 +2040,23 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
>  
>  	sfp->id = id;
>  
> +	sfp->phy_addr = SFP_PHY_ADDR;
> +
> +	rollball = ((!memcmp(id.base.vendor_name, "OEM             ", 16) ||
> +		     !memcmp(id.base.vendor_name, "Turris          ", 16)) &&
> +		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-10", 8)));

Are you customising the SFP, so that it has your vendor name?

Is the generic SFP OEM/SFP-10G-T, and your customized one Turris/ 
RTSFP-10?

	Andrew
Andrew Lunn Oct. 20, 2020, 4 p.m. UTC | #2 
> This extends the mdio-i2c driver so that when SFP PHY address 17 is used
> (which in mdio-i2c terms corresponds to I2C address 0x51), then this
> different protocol is used for MDIO access.

Hi Marek

I don't see that being very scalable. What happens when the next SFP
comes along which has a different protocol at address 0x51. Since you
can identify the SFP via the EEPROM information, i would prefer you
explicitly tell it to use the rollball protocol when instantiating the
MDIO bus.

>   * I2C bus addresses 0x50 and 0x51 are normally an EEPROM, which is
>   * specified to be present in SFP modules.  These correspond with PHY
> - * addresses 16 and 17.  Disallow access to these "phy" addresses.
> + * addresses 16 and 17.  Disallow access to 0x50 "phy" address.
> + * Use RollBall protocol when accessing via the 0x51 address.
>   */
>  static bool i2c_mii_valid_phy_id(int phy_id)
>  {
> -	return phy_id != 0x10 && phy_id != 0x11;
> +	return phy_id != 0x10;
> +}

I'm not sure that is safe. It means that we will scan address 0x11 to
see if there is a PHY there. And if the SFP does have diagnostics
registers, that might be enough that phylib thinks there is a PHY
there.

I think you only need to allow access to 0x11 if rollball protocol has
been enabled.

     Andrew
Marek Behún Oct. 20, 2020, 4:14 p.m. UTC | #3 
On Tue, 20 Oct 2020 17:51:26 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > @@ -2006,6 +2040,23 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)

> >  

> >  	sfp->id = id;

> >  

> > +	sfp->phy_addr = SFP_PHY_ADDR;

> > +

> > +	rollball = ((!memcmp(id.base.vendor_name, "OEM             ", 16) ||

> > +		     !memcmp(id.base.vendor_name, "Turris          ", 16)) &&

> > +		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||

> > +		     !memcmp(id.base.vendor_pn, "RTSFP-10", 8)));  

> 

> Are you customising the SFP, so that it has your vendor name?

> 

> Is the generic SFP OEM/SFP-10G-T, and your customized one Turris/ 

> RTSFP-10?

> 

> 	Andrew


Hilink puts OEM/SFP-10G-T into their modules.
RollBall puts OEM/RTSFP-10 and sometimes OEM/RTSFP-10G.
They are rebranding these modules for us to Turris/RTSFP-10.

Marek
Marek Behún Oct. 20, 2020, 4:15 p.m. UTC | #4 
On Tue, 20 Oct 2020 18:00:39 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > This extends the mdio-i2c driver so that when SFP PHY address 17 is used
> > (which in mdio-i2c terms corresponds to I2C address 0x51), then this
> > different protocol is used for MDIO access.  
> 
> Hi Marek
> 
> I don't see that being very scalable. What happens when the next SFP
> comes along which has a different protocol at address 0x51. Since you
> can identify the SFP via the EEPROM information, i would prefer you
> explicitly tell it to use the rollball protocol when instantiating the
> MDIO bus.

At first I proposed a separate mdio bus driver for RollBall SFPs.
But Russell suggested doing this instead, saying that in the future
this can be changed.

> 
> >   * I2C bus addresses 0x50 and 0x51 are normally an EEPROM, which is
> >   * specified to be present in SFP modules.  These correspond with PHY
> > - * addresses 16 and 17.  Disallow access to these "phy" addresses.
> > + * addresses 16 and 17.  Disallow access to 0x50 "phy" address.
> > + * Use RollBall protocol when accessing via the 0x51 address.
> >   */
> >  static bool i2c_mii_valid_phy_id(int phy_id)
> >  {
> > -	return phy_id != 0x10 && phy_id != 0x11;
> > +	return phy_id != 0x10;
> > +}  
> 
> I'm not sure that is safe. It means that we will scan address 0x11 to
> see if there is a PHY there. And if the SFP does have diagnostics
> registers, that might be enough that phylib thinks there is a PHY
> there.
> 
> I think you only need to allow access to 0x11 if rollball protocol has
> been enabled.

I can do that...

> 
>      Andrew