On Thursday 14 January 2021 16:07:19 Russell King - ARM Linux admin wrote:

On Thu, Jan 14, 2021 at 05:43:30AM +0100, Marek Behún wrote:

quoted

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?

Hello Russell! For me this solution looks more cleaner. As all those
MDIO access protocols are vendor dependent, kernel code should not
detect them only from the standard (non-vendor) extended_cc property.