On Thu, Jan 14, 2021 at 05:43:30AM +0100, Marek Behún wrote:
> Instead of configuring the I2C mdiobus when SFP driver is probed,
> create/destroy the mdiobus before the PHY is probed for/after it is
> released.
>
> This way we can tell the mdio-i2c code which protocol to use for each
> SFP transceiver.
I've been thinking a bit more about this. It looks like it will
allocate and free the MDIO bus each time any module is inserted or
removed, even a fiber module that wouldn't ever have a PHY. This adds
unnecessary noise to the kernel message log.
We only probe for a PHY if one of:
- id.base.extended_cc is SFF8024_ECC_10GBASE_T_SFI,
SFF8024_ECC_10GBASE_T_SR, SFF8024_ECC_5GBASE_T, or
SFF8024_ECC_2_5GBASE_T.
- id.base.e1000_base_t is set.
So, we only need the MDIO bus to be registered if one of those is true.
As you are introducing "enum mdio_i2c_proto", I'm wondering whether
that should include "MDIO_I2C_NONE", and we should only register the
bus and probe for a PHY if it is not MDIO_I2C_NONE.
Maybe we should have:
enum mdio_i2c_proto {
MDIO_I2C_NONE,
MDIO_I2C_MARVELL_C22,
MDIO_I2C_C45,
MDIO_I2C_ROLLBALL,
...
};
with:
sfp->mdio_protocol = MDIO_I2C_NONE;
if (((!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)))) {
sfp->mdio_protocol = MDIO_I2C_ROLLBALL;
sfp->module_t_wait = T_WAIT_ROLLBALL;
} else {
switch (id.base.extended_cc) {
...
}
}
static int sfp_sm_add_mdio_bus(struct sfp *sfp)
{
int err = 0;
if (sfp->mdio_protocol != MDIO_I2C_NONE)
err = sfp_i2c_mdiobus_create(sfp);
return err;
}
called from the place you call sfp_i2c_mdiobus_create(), and
sfp_sm_probe_for_phy() becomes:
static int sfp_sm_probe_for_phy(struct sfp *sfp)
{
int err = 0;
switch (sfp->mdio_protocol) {
case MDIO_I2C_NONE:
break;
case MDIO_I2C_MARVELL_C22:
err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, false);
break;
case MDIO_I2C_C45:
err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, true);
break;
case MDIO_I2C_ROLLBALL:
err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
break;
}
return err;
}
This avoids having to add the PHY address, as well as fudge around with
id.base.extended_cc to get the PHY probed.
Thoughts?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
