| Message ID | 20210114044331.5073-1-kabel@kernel.org |
|---|---|
| Headers | show |
| Series | Support for RollBall 10G copper SFP modules | expand |
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: > > 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.
On Mon, 18 Jan 2021 10:38:32 +0100 Pali Rohár <pali@kernel.org> wrote: > 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: > > > 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. I shall respin this series with this modified, then. Marek
Hello, this is v5 of series adding support for RollBall/Hilink SFP modules. Checked with: checkpatch.pl --max-line-length=80 Changes from v4: - added patch adding SFP_PASSWORD = 0x7b to sfp.h - patch adding RollBall MDIO I2C protocol support - removed forgotten comment that was no longer true - we now try to restore the page if the I2C transfer failed - unfortunately we cannot simply move the code to do paged I2C transfers to sfp.c, because sfp.c depends on code inmdio-i2c.c, and if we added dependency the other way, I don't know what would happen to module loader (since both SFP and MDIO_I2C can be compiled as modules). This can be refactored later, when someone needs paged I2C accesses in sfp.c - added Reviewed-by tags Changes from v3: - RollBall mdio-i2c driver now sets/restores SFP_PAGE for every MDIO access. I first wanted to achieve this operation (setting SFP_PAGE/doing MDIO/restoring SFP_PAGE) via one call do i2c_transfer, by constructing msgs array in such a way, but it turned out that this doesn't work on RollBall SFPs, because changed SFP_PAGE takes into account only after i2c_transfer ends. So instead I use i2c_lock_bus/serveral __i2c_transfers/i2c_unlock_bus. - I have removed the patch which changes MACTYPE in the marvell10g driver, since Russell has in his net-queue a better solution. I still think that my patch would have sufficed temporarily (and would not cause regressions), but nobody wanted to review it. If you think that I should sent this series again with that patch, please let me know. Changes from v2: - added comment into the patch adding support for RollBall I2C MDIO protocol, saying that we expect the SFP_PAGE not to be changed by the SFP code, as requested by Russell. If, in the future, SFP code starts modifying SFP_PAGE, we will have to handle it in mdio-i2c somehow - destruction of I2C MDIO bus in patch 3/5 now depends on whether the MDIO bus is not NULL, instead of whether PHY exists, as suggested by Russell - changed waiting time for RollBall module to initialize from 30 seconds to 25 seconds. Testing shows that it is never longer than 21-22 seconds, so waiting 25 seconds instead of 30 is IMO safe enough - added Russell's Reviewed-by tags where relevant Changes from v1: - wrapped to 80 columns as per Russell's request - initialization of RollBall MDIO I2C protocol moved from sfp.c to mdio-i2c.c as per Russell's request - second patch removes the 802.3z check also from phylink_sfp_config as suggested by Russell - creation/destruction of mdiobus for SFP now occurs before probing for PHY/after releasing PHY (as suggested by Russell) - the last patch became a little simpler after the above was done Marek Marek Behún (5): net: sfp: add SFP_PASSWORD address net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules net: phylink: allow attaching phy for SFP modules on 802.3z mode net: sfp: create/destroy I2C mdiobus before PHY probe/after PHY release net: sfp: add support for multigig RollBall transceivers drivers/net/mdio/mdio-i2c.c | 308 +++++++++++++++++++++++++++++++++- drivers/net/phy/phylink.c | 5 +- drivers/net/phy/sfp.c | 66 ++++++-- include/linux/mdio/mdio-i2c.h | 8 +- include/linux/sfp.h | 1 + 5 files changed, 368 insertions(+), 20 deletions(-) base-commit: 0ae5b43d6dde6003070106e97cd0d41bace2eeb2